| Message ID | 20170808230459.GF29372@obsidianresearch.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Le 09/08/2017 à 01:04, Jason Gunthorpe a écrit : > On Fri, Aug 04, 2017 at 06:59:10PM +0200, Nicolas Morey-Chaisemartin wrote: >> Generate a temporary file with the spec file parsed and all its macro replaced. >> Avoid manually replacing Version and handle extra macros >> >> Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin@suse.com> >> .gitignore | 3 +++ >> buildlib/cbuild | 8 ++++++-- >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/.gitignore b/.gitignore >> index f31a0617..52c7af2f 100644 >> +++ b/.gitignore >> @@ -69,3 +69,6 @@ tramp >> Session.vim >> .netrwhist >> *~ >> + >> +# rpm >> +*/*.spec.processed >> \ No newline at end of file >> diff --git a/buildlib/cbuild b/buildlib/cbuild >> index 5228ec1b..cf7828eb 100755 >> +++ b/buildlib/cbuild >> @@ -404,7 +404,12 @@ def get_image_id(args,image_name): >> >> def run_rpm_build(args,spec_file,env): >> version = get_version(); >> - with open(spec_file,"r") as F: >> + >> + # Pre-process file with rpm spec to avoid manually replacing macros >> + f = open(spec_file + ".processed", "w"); >> + subprocess.check_call(["rpmspec", "-P", spec_file], stdout=f); > This isn't going to work on any host system that does not have > rpmspec, including my Debian systems.. Are you building RPM outside of docker on your Debian ? I ran the pkg suite for all targets and they all work. rpmspec is with RPM since v4.9.0 which came out in early 2011 so it should work with all current RPM -based distro > How about something like this instead for the two patches? > > From 13e239b5fc39fff9e56760969b99eab462db3d89 Mon Sep 17 00:00:00 2001 > From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Date: Tue, 8 Aug 2017 17:04:06 -0600 > Subject: [PATCH] cbuild: Be more robust when computing path names > > Always use the same format for the git archvie prefix, this matches > the format that github-release uses, and is what a .spec file must work > with. > > Use rpmspec -P to determine what the expected tar file name is instead of > trying to do substitution internally. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > buildlib/cbuild | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/buildlib/cbuild b/buildlib/cbuild > index 9b49fa5343c8b4..a3cf26e85bc722 100755 > --- a/buildlib/cbuild > +++ b/buildlib/cbuild > @@ -409,16 +409,10 @@ def get_image_id(args,image_name): > # ------------------------------------------------------------------------- > > def run_rpm_build(args,spec_file,env): > - version = get_version(); > with open(spec_file,"r") as F: > for ln in F: > - if ln.startswith("Version:"): > - ver = ln.strip().partition(' ')[2]; > - assert(ver == get_version()); > - You are losing this last check which seems important. It make sure the ti tag is in sync with the RPM > if ln.startswith("Source:"): > tarfn = ln.strip().partition(' ')[2]; > - tarfn = tarfn.replace("%{version}",version); > > image_id = get_image_id(args,env.image_name()); > with private_tmp(args) as tmpdir: > @@ -426,7 +420,8 @@ def run_rpm_build(args,spec_file,env): > os.mkdir(os.path.join(tmpdir,"tmp")); > > subprocess.check_call(["git","archive", > - "--prefix","%s/"%(os.path.splitext(tarfn)[0]), > + # This must match the prefix generated buildlib/github-release > + "--prefix","%s-%s/"%(project,get_version()), > "--output",os.path.join(tmpdir,"SOURCES",tarfn), You have an issue here because tarfn is not set yet. I'm confused on why moving the call to rpmspec further down would solve your Debian issue ? > "HEAD"]); > > @@ -453,17 +448,25 @@ def run_rpm_build(args,spec_file,env): > # for the user we are going to use to do the build. > with open(os.path.join(tmpdir,"go.py"),"w") as F: > print >> F,""" > -import os; > +import os,subprocess; > with open("/etc/passwd","a") as F: > print >> F, {passwd!r}; > with open("/etc/group","a") as F: > print >> F, {group!r}; > os.setgid({gid:d}); > os.setuid({uid:d}); > + > +# Get RPM to tell us the expected tar filename. > +for ln in subprocess.check_output(["rpmspec","-P",{tspec_file!r}]).splitlines(): > + if ln.startswith("Source:"): > + tarfn = ln.strip().partition(' ')[2]; > +os.symlink({tarfn!r},os.path.join("SOURCES",tarfn)); > """.format(passwd=":".join(str(I) for I in pwd.getpwuid(os.getuid())), > group=":".join(str(I) for I in grp.getgrgid(os.getgid())), > uid=os.getuid(), > - gid=os.getgid()); > + gid=os.getgid(), > + tarfn=tarfn, > + tspec_file=tspec_file); > > bopts = ["-bb",tspec_file]; > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 09, 2017 at 11:56:47AM +0200, Nicolas Morey-Chaisemartin wrote: > >> def run_rpm_build(args,spec_file,env): > >> version = get_version(); > >> - with open(spec_file,"r") as F: > >> + > >> + # Pre-process file with rpm spec to avoid manually replacing macros > >> + f = open(spec_file + ".processed", "w"); > >> + subprocess.check_call(["rpmspec", "-P", spec_file], stdout=f); > > This isn't going to work on any host system that does not have > > rpmspec, including my Debian systems.. > Are you building RPM outside of docker on your Debian ? No.. The issue is the above subprocess.check_call is run on the host, while the call I added below inside go.py is run inside the container. The host may not have rpmspec, while we guarentee that the container does. > I ran the pkg suite for all targets and they all work. Your host has rpmspec :) > > def run_rpm_build(args,spec_file,env): > > - version = get_version(); > > with open(spec_file,"r") as F: > > for ln in F: > > - if ln.startswith("Version:"): > > - ver = ln.strip().partition(' ')[2]; > > - assert(ver == get_version()); > > - > You are losing this last check which seems important. It make sure the ti tag is in sync with the RPM If you think it is important it can be left in.. > > if ln.startswith("Source:"): > > tarfn = ln.strip().partition(' ')[2]; > > - tarfn = tarfn.replace("%{version}",version); > > > > image_id = get_image_id(args,env.image_name()); > > with private_tmp(args) as tmpdir: > > @@ -426,7 +420,8 @@ def run_rpm_build(args,spec_file,env): > > os.mkdir(os.path.join(tmpdir,"tmp")); > > > > subprocess.check_call(["git","archive", > > - "--prefix","%s/"%(os.path.splitext(tarfn)[0]), > > + # This must match the prefix generated buildlib/github-release > > + "--prefix","%s-%s/"%(project,get_version()), > > "--output",os.path.join(tmpdir,"SOURCES",tarfn), > You have an issue here because tarfn is not set yet. No, it was still set above, see the first hunk. > I'm confused on why moving the call to rpmspec further down would solve your Debian issue ? Further down the code is run in the container where rpmspec is guarenteed to exist. Everything in cbuild is run on the host except for the snippet in go.py Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 09/08/2017 à 17:55, Jason Gunthorpe a écrit : > On Wed, Aug 09, 2017 at 11:56:47AM +0200, Nicolas Morey-Chaisemartin wrote: > >>>> def run_rpm_build(args,spec_file,env): >>>> version = get_version(); >>>> - with open(spec_file,"r") as F: >>>> + >>>> + # Pre-process file with rpm spec to avoid manually replacing macros >>>> + f = open(spec_file + ".processed", "w"); >>>> + subprocess.check_call(["rpmspec", "-P", spec_file], stdout=f); >>> This isn't going to work on any host system that does not have >>> rpmspec, including my Debian systems.. >> Are you building RPM outside of docker on your Debian ? > No.. > > The issue is the above subprocess.check_call is run on the host, while > the call I added below inside go.py is run inside the container. > > The host may not have rpmspec, while we guarentee that the container > does. Oh OK. I dived into those specific bits of code and assume it was all running in the container. > >> I ran the pkg suite for all targets and they all work. > Your host has rpmspec :) Yes :) > >>> def run_rpm_build(args,spec_file,env): >>> - version = get_version(); >>> with open(spec_file,"r") as F: >>> for ln in F: >>> - if ln.startswith("Version:"): >>> - ver = ln.strip().partition(' ')[2]; >>> - assert(ver == get_version()); >>> - >> You are losing this last check which seems important. It make sure the ti tag is in sync with the RPM > If you think it is important it can be left in.. Well as we will have 2 spec file, it makes sure no ones forgot a version bump when releasing late at night just before a deadline ;) > >>> if ln.startswith("Source:"): >>> tarfn = ln.strip().partition(' ')[2]; >>> - tarfn = tarfn.replace("%{version}",version); >>> >>> image_id = get_image_id(args,env.image_name()); >>> with private_tmp(args) as tmpdir: >>> @@ -426,7 +420,8 @@ def run_rpm_build(args,spec_file,env): >>> os.mkdir(os.path.join(tmpdir,"tmp")); >>> >>> subprocess.check_call(["git","archive", >>> - "--prefix","%s/"%(os.path.splitext(tarfn)[0]), >>> + # This must match the prefix generated buildlib/github-release >>> + "--prefix","%s-%s/"%(project,get_version()), >>> "--output",os.path.join(tmpdir,"SOURCES",tarfn), >> You have an issue here because tarfn is not set yet. > No, it was still set above, see the first hunk. Yes. But the value it has is "wrong" If I take the current spec file used in OBS: Source0: %{name}-%{version}%{git_ver}.tar.xz Omitting the Source0 that should be changed to Source, this wille set tarfn to rdma-core-15%{git_ver}.tar.xz > >> I'm confused on why moving the call to rpmspec further down would solve your Debian issue ? > Further down the code is run in the container where rpmspec is > guarenteed to exist. Everything in cbuild is run on the host except > for the snippet in go.py > > Jason This means that we would need to detar/rename/retar the package in the go.py. It feels overly complicated :/ What we use this for is mainly for RC and non-release packages. We set the git_ver to -rc1, -<NN>-g<SHA1> so we know at a glance that this is not an official release. I can either try to get this working in the go.py, or I'll simply drop this from our spec and bring it back when needed for non release packages. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 09, 2017 at 06:39:23PM +0200, Nicolas Morey-Chaisemartin wrote: > >>> subprocess.check_call(["git","archive", > >>> - "--prefix","%s/"%(os.path.splitext(tarfn)[0]), > >>> + # This must match the prefix generated buildlib/github-release > >>> + "--prefix","%s-%s/"%(project,get_version()), > >>> "--output",os.path.join(tmpdir,"SOURCES",tarfn), > >> You have an issue here because tarfn is not set yet. > > No, it was still set above, see the first hunk. > > Yes. But the value it has is "wrong" > If I take the current spec file used in OBS: > Source0: %{name}-%{version}%{git_ver}.tar.xz > > Omitting the Source0 that should be changed to Source, this wille set tarfn to rdma-core-15%{git_ver}.tar.xz Okay.. Why not use %setup -n %{name}-%{version} In the spec file? Eg do not include the git version in the prefix so it matches the format produced by the github scripts. Then, the above will make a archive with 'rdma-core-15/' as the prefix. The archive will be called 'SOURCE/%{name}-%{version}%{git_ver}.tar.xz' Then the go.py will parse and determine that the archive should be called 'SOURCE/rdma-core-15-git124556.tar.xz' And drop a symlink to fix it up. > This means that we would need to detar/rename/retar the package in > the go.py. It feels overly complicated :/ What we use this for is > mainly for RC and non-release packages. We set the git_ver to -rc1, > -<NN>-g<SHA1> so we know at a glance that this is not an official > release. I can either try to get this working in the go.py, or I'll > simply drop this from our spec and bring it back when needed for non > release packages. I think if this is your usage then you would be fine to use '%setup -n'.. We can already recover the git HEAD from a git archive output by using 'git get-tar-commit-id' so it isn't really necessary to include the git commit id in the prefix. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 09/08/2017 à 22:19, Jason Gunthorpe a écrit : > On Wed, Aug 09, 2017 at 06:39:23PM +0200, Nicolas Morey-Chaisemartin wrote: >>>>> subprocess.check_call(["git","archive", >>>>> - "--prefix","%s/"%(os.path.splitext(tarfn)[0]), >>>>> + # This must match the prefix generated buildlib/github-release >>>>> + "--prefix","%s-%s/"%(project,get_version()), >>>>> "--output",os.path.join(tmpdir,"SOURCES",tarfn), >>>> You have an issue here because tarfn is not set yet. >>> No, it was still set above, see the first hunk. >> Yes. But the value it has is "wrong" >> If I take the current spec file used in OBS: >> Source0: %{name}-%{version}%{git_ver}.tar.xz >> >> Omitting the Source0 that should be changed to Source, this wille set tarfn to rdma-core-15%{git_ver}.tar.xz > Okay.. > > Why not use > > %setup -n %{name}-%{version} > > In the spec file? Eg do not include the git version in the prefix so > it matches the format produced by the github scripts. > > Then, the above will make a archive with 'rdma-core-15/' as the prefix. > > The archive will be called > > 'SOURCE/%{name}-%{version}%{git_ver}.tar.xz' > > Then the go.py will parse and determine that the archive should be > called > > 'SOURCE/rdma-core-15-git124556.tar.xz' > > And drop a symlink to fix it up. Yes this should work. It feels a little bit dirty to have a tarball name with unreplaced macro but it's a temporay state so I can live with this :) >> This means that we would need to detar/rename/retar the package in >> the go.py. It feels overly complicated :/ What we use this for is >> mainly for RC and non-release packages. We set the git_ver to -rc1, >> -<NN>-g<SHA1> so we know at a glance that this is not an official >> release. I can either try to get this working in the go.py, or I'll >> simply drop this from our spec and bring it back when needed for non >> release packages. > I think if this is your usage then you would be fine to use '%setup > -n'.. > > We can already recover the git HEAD from a git archive output by using > 'git get-tar-commit-id' so it isn't really necessary to include the > git commit id in the prefix. > > Jason Yes the setup part is easy enough. There two reason for this: - I have a lot of packages to maintain and I can see at a glance what is what. - If someone wants to fork the package in OBS and have its own build (for whatever reason: different compiler, different flag, extra patches, etc.) it clears which exact version it is without digging the SHA1 from the _service file we have or within the tarball. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 09, 2017 at 11:08:34PM +0200, Nicolas Morey-Chaisemartin wrote: > > And drop a symlink to fix it up. > Yes this should work. It feels a little bit dirty to have a tarball > name with unreplaced macro but it's a temporay state so I can live > with this :) It is a bit strange, but this trivially delegates parsing the .tar.xx to git archive which understands all the formats.. Here is the patches, should I send them? https://github.com/jgunthorpe/rdma-plumbing/tree/cbuild Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 09, 2017 at 03:15:36PM -0600, Jason Gunthorpe wrote: > On Wed, Aug 09, 2017 at 11:08:34PM +0200, Nicolas Morey-Chaisemartin wrote: > > > > And drop a symlink to fix it up. > > > Yes this should work. It feels a little bit dirty to have a tarball > > name with unreplaced macro but it's a temporay state so I can live > > with this :) > > It is a bit strange, but this trivially delegates parsing the .tar.xx > to git archive which understands all the formats.. > > Here is the patches, should I send them? > > https://github.com/jgunthorpe/rdma-plumbing/tree/cbuild ???? Thanks
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Le 13/08/2017 à 08:37, Leon Romanovsky a écrit : > On Wed, Aug 09, 2017 at 03:15:36PM -0600, Jason Gunthorpe wrote: >> On Wed, Aug 09, 2017 at 11:08:34PM +0200, Nicolas Morey-Chaisemartin wrote: >> >>>> And drop a symlink to fix it up. >> >>> Yes this should work. It feels a little bit dirty to have a tarball >>> name with unreplaced macro but it's a temporay state so I can live >>> with this :) >> >> It is a bit strange, but this trivially delegates parsing the .tar.xx >> to git archive which understands all the formats.. >> >> Here is the patches, should I send them? >> >> https://github.com/jgunthorpe/rdma-plumbing/tree/cbuild > > ???? > > Thanks > Sorry for the late reply, was out of the office. Yes your patches look good to me. Thanks ! Nicolas (Resent as previous one got dropped by majordomo due to HTML) -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBAgAGBQJZmpFZAAoJEIAb3bglmI9kIxYH/3hfV0ep7UxlpU7uMay2OUAh X1V9rwikyMoTkI9qDDmVDpBL0WpUYVSSkAZio00a24I2HUVa4+ZIj6d27EYZkRru oFloEkfH+aER/KvVkfWjE8SYOzS31ed6cgDRlqqjEncehobYR630Q3g/zfRpml+V aoJtMakrHrZBAcnqD2XVxieaD4KOjx4IaewrKrV+hLxLuw6UjZHtbaKAD/HHajWs I1xUQTHaIMHO6Su982+pTDyY0zlcK2MtzsPK2gcxuyiGn3oHAd9k6xpfsxXiWa5M 6LV60V/O/6QXDu/CAok/DP+E+zg0RgoHoftVnkGkS4zR3b0r1RZDUuxb8XWloL0= =0XFR -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 21, 2017 at 09:52:57AM +0200, Nicolas Morey-Chaisemartin wrote: > Sorry for the late reply, was out of the office. > Yes your patches look good to me. https://github.com/linux-rdma/rdma-core/pull/186 Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/buildlib/cbuild b/buildlib/cbuild index 9b49fa5343c8b4..a3cf26e85bc722 100755 --- a/buildlib/cbuild +++ b/buildlib/cbuild @@ -409,16 +409,10 @@ def get_image_id(args,image_name): # ------------------------------------------------------------------------- def run_rpm_build(args,spec_file,env): - version = get_version(); with open(spec_file,"r") as F: for ln in F: - if ln.startswith("Version:"): - ver = ln.strip().partition(' ')[2]; - assert(ver == get_version()); - if ln.startswith("Source:"): tarfn = ln.strip().partition(' ')[2]; - tarfn = tarfn.replace("%{version}",version); image_id = get_image_id(args,env.image_name()); with private_tmp(args) as tmpdir: @@ -426,7 +420,8 @@ def run_rpm_build(args,spec_file,env): os.mkdir(os.path.join(tmpdir,"tmp")); subprocess.check_call(["git","archive", - "--prefix","%s/"%(os.path.splitext(tarfn)[0]), + # This must match the prefix generated buildlib/github-release + "--prefix","%s-%s/"%(project,get_version()), "--output",os.path.join(tmpdir,"SOURCES",tarfn), "HEAD"]); @@ -453,17 +448,25 @@ def run_rpm_build(args,spec_file,env): # for the user we are going to use to do the build. with open(os.path.join(tmpdir,"go.py"),"w") as F: print >> F,""" -import os; +import os,subprocess; with open("/etc/passwd","a") as F: print >> F, {passwd!r}; with open("/etc/group","a") as F: print >> F, {group!r}; os.setgid({gid:d}); os.setuid({uid:d}); + +# Get RPM to tell us the expected tar filename. +for ln in subprocess.check_output(["rpmspec","-P",{tspec_file!r}]).splitlines(): + if ln.startswith("Source:"): + tarfn = ln.strip().partition(' ')[2]; +os.symlink({tarfn!r},os.path.join("SOURCES",tarfn)); """.format(passwd=":".join(str(I) for I in pwd.getpwuid(os.getuid())), group=":".join(str(I) for I in grp.getgrgid(os.getgid())), uid=os.getuid(), - gid=os.getgid()); + gid=os.getgid(), + tarfn=tarfn, + tspec_file=tspec_file); bopts = ["-bb",tspec_file];