Hi,
I think the `preferred buildroot' is not really making sense. The above has developed historically out of a misunderstanding in ancient buildroot times.
When people were building as root and BuildRoots were defined as %{_tmppath}/%{name}-%{version}-root, some considered "root" to mean the uid of the builder. Later %release was added and some replaced root with `id -un`. Even later some realized that root was referring to the BuildRoot and in order to play safe added both.
I'm using %{_tmppath}/%{name}-%{version}-%{release}-root in my packages and someone is now nitpicking on why not using the preferred BuildRoot as given in the guidelines. Instead of locally fighting a BuildRoot battle, I'd better get the guidelines fixed ;)
Also consider what this really is about: Deambiguifying the BuildRoot per package makes sense as there may be several build processes sharing the same filesystem (either locally or through NFS), but deambiguifying the build user, too, means that we assume that the same EVR package will be possibly built on the same filesystem by different users? And even simultaneously?
It makes more sense to include a conditional epoch or target/arch in the buildroot that the builder. In fact the best thing for a buildsystem is to override the buildroot adding a build-id to it anyway.
On Wednesday 19 July 2006 08:29, Axel Thimm wrote:
It makes more sense to include a conditional epoch or target/arch in the buildroot that the builder. In fact the best thing for a buildsystem is to override the buildroot adding a build-id to it anyway.
Or what we REALLY should do is have rpm(build) supplant a standard buildroot when one isn't found in the spec file, so we can REMOVE Buildroot from the spec file all together and no longer have these discussions. Instead of nitpicking on how the buildroot should look, we just say 'remove buildroot definitions'. That's KISS.
On Wed, Jul 19, 2006 at 08:41:45AM -0400, Jesse Keating wrote:
On Wednesday 19 July 2006 08:29, Axel Thimm wrote:
It makes more sense to include a conditional epoch or target/arch in the buildroot that the builder. In fact the best thing for a buildsystem is to override the buildroot adding a build-id to it anyway.
Or what we REALLY should do is have rpm(build) supplant a standard buildroot when one isn't found in the spec file, so we can REMOVE Buildroot from the spec file all together and no longer have these discussions. Instead of nitpicking on how the buildroot should look, we just say 'remove buildroot definitions'. That's KISS.
I would agree, if it weren't for undefined behaviour at best when someone uses the buildroot-less specfile on a system not supplying a default buildroot.
In the worst case you could end up with an empty buildroot and %install/%clean operations on the buildroot could suddenly really happen in the live filesystem.
So, even if we get some current/future rpm version to behave as we wish it we would need to allow for many years to pass to really start dropping BuildRoot tags.
On Wednesday 19 July 2006 08:51, Axel Thimm wrote:
I would agree, if it weren't for undefined behaviour at best when someone uses the buildroot-less specfile on a system not supplying a default buildroot.
In the worst case you could end up with an empty buildroot and %install/%clean operations on the buildroot could suddenly really happen in the live filesystem.
Have you tried (within a chroot) to build a package w/out a chroot on our modern rpm? (I haven't yet, other things to do today...)
On Wed, Jul 19, 2006 at 09:10:16AM -0400, Jesse Keating wrote:
On Wednesday 19 July 2006 08:51, Axel Thimm wrote:
I would agree, if it weren't for undefined behaviour at best when someone uses the buildroot-less specfile on a system not supplying a default buildroot.
In the worst case you could end up with an empty buildroot and %install/%clean operations on the buildroot could suddenly really happen in the live filesystem.
Have you tried (within a chroot) to build a package w/out a chroot on our modern rpm? (I haven't yet, other things to do today...)
You mean w/out a buildroot?
I did try on FC5, and there
BuildRoot: foo
is effectively the same as
%define buildroot foo
No errors/warnings if BuildRoot is missing (and of course no sensible default, in fact no definition of the buildroot macro at all), which is a bit disappointing if you ask me.
But that's only half the story. Even if we would find FC5's rpm to do something sensible w/o a BuildRoot tag, we cannot rely only on modern rpm's behaviour and try to check whether a missing BuildRoot tag would create havoc there or not. You would need to check this behaviour with past rpm versions, both packaged in Red Hat/Fedora products/releases and even upstream. We don't want to be responsible for any breakage, not even outside Fedora/RHEL.
I don't think it's worth while risking it. Better/easier to
a) override it in buildsystems aynway, and b) put a sensible default in the guidelines
Axel Thimm wrote:
On Wed, Jul 19, 2006 at 09:10:16AM -0400, Jesse Keating wrote:
I don't think it's worth while risking it. Better/easier to
a) override it in buildsystems aynway, and
It's been tossed around that this might be something worth stuffing into redhat-rpm-config
b) put a sensible default in the guidelines
IMO, the guidelines already include a sensible default.
-- Rex
On Wed, Jul 19, 2006 at 08:35:53AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
On Wed, Jul 19, 2006 at 09:10:16AM -0400, Jesse Keating wrote:
I don't think it's worth while risking it. Better/easier to
a) override it in buildsystems aynway, and
It's been tossed around that this might be something worth stuffing into redhat-rpm-config
b) put a sensible default in the guidelines
IMO, the guidelines already include a sensible default.
Well, the thread instantly drifted from discussing the default to discussing removing it altogether. Unfortunately the arguments are only found at the beginning of the thread, so you may want to check that. In short id -un doesn't make sense, even epoch or target/arch would make more far more sense in a guideline's BuildRoot.
Note that the guidelines are also there to educate people how to write clean and non-obfuscated specfiles. I'm quite sure the BuildRoot is cut & pasted in 99.99% of the packages making it a defacto proper thing to do. If it's bogus we need to fix it and not endorse it furthermore. Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:35:53AM -0500, Rex Dieter wrote:
b) put a sensible default in the guidelines
IMO, the guidelines already include a sensible default.
In short id -un doesn't make sense, even epoch or target/arch would make more far more sense in a guideline's BuildRoot.
Note that the guidelines are also there to educate people how to write clean and non-obfuscated specfiles. I'm quite sure the BuildRoot is cut & pasted in 99.99% of the packages making it a defacto proper thing to do. If it's bogus we need to fix it and not endorse it furthermore.
It's simply my opinion that it's not worth fixing something that isn't broken.
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
-- Rex
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:35:53AM -0500, Rex Dieter wrote:
b) put a sensible default in the guidelines
IMO, the guidelines already include a sensible default.
In short id -un doesn't make sense, even epoch or target/arch would make more far more sense in a guideline's BuildRoot.
Note that the guidelines are also there to educate people how to write clean and non-obfuscated specfiles. I'm quite sure the BuildRoot is cut & pasted in 99.99% of the packages making it a defacto proper thing to do. If it's bogus we need to fix it and not endorse it furthermore.
It's simply my opinion that it's not worth fixing something that isn't broken.
It's not broken as in "fix all packages that use id -un", it just shouldn't be promoted anymore as a default since it makes no sense. And given that it does create workload on packager and reviewer everytime this will come up again, let's fix it.
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
That's what I meant with "wrong education". I can't blame reviewers for following the guidelines to the letter (and since they are scarce, I also don't want to use any punishment on them ;).
I thought this would be an EasyFix. :/
On Wed, 2006-07-19 at 16:15 +0200, Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:35:53AM -0500, Rex Dieter wrote:
b) put a sensible default in the guidelines
IMO, the guidelines already include a sensible default.
In short id -un doesn't make sense, even epoch or target/arch would make more far more sense in a guideline's BuildRoot.
Note that the guidelines are also there to educate people how to write clean and non-obfuscated specfiles. I'm quite sure the BuildRoot is cut & pasted in 99.99% of the packages making it a defacto proper thing to do. If it's bogus we need to fix it and not endorse it furthermore.
It's simply my opinion that it's not worth fixing something that isn't broken.
It's not broken as in "fix all packages that use id -un", it just shouldn't be promoted anymore as a default since it makes no sense. And given that it does create workload on packager and reviewer everytime this will come up again, let's fix it.
Axel, put the exact buildroot that you want to change the guidelines to use on the todo agenda for tomorrow, and we'll vote on it.
~spot
On Wed, Jul 19, 2006 at 09:37:25AM -0500, Tom 'spot' Callaway wrote:
Axel, put the exact buildroot that you want to change the guidelines to use on the todo agenda for tomorrow, and we'll vote on it.
I'm not sure I'd like to waste IRC time on this. At first I thought most people would second that and that it would be a trivial item. As it stands a vote could require further discussion and we'd rather keep our IRC time for more valued items.
The only thing would be a clarification of the wording that the "preferred buildroot" as quoted in the guidelines is a SHOULD item as the word "preferred" implies. I don't want to fight this battle with each review on every package I submit.
On Wednesday 19 July 2006 18:08, Axel Thimm wrote:
The only thing would be a clarification of the wording that the "preferred buildroot" as quoted in the guidelines is a SHOULD item as the word "preferred" implies. I don't want to fight this battle with each review on every package I submit.
Sure, it's a should. I still say "Buildroot SHOULD be <foo>" even if they have a Buildroot defined. Its their choice to follow the advice. I don't block on it (anymore).
On Wed, 2006-07-19 at 16:15 +0200, Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
The point you seem to be missing, your buildroot is broken: buildroot: %{_tmppath}/%{name}-%{version}-%{release}-root
This directory is NOT unique and will break if 2 or more users are running an rpmbuild in parallel on the same /var/tmp filesystem.
It will also break if 2 different users are running buildjobs of the same package consecutively and if the first one fails and leaves it buildroot behind.
Ralf
Ralf Corsepius wrote:
On Wed, 2006-07-19 at 16:15 +0200, Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
The point you seem to be missing, your buildroot is broken: buildroot: %{_tmppath}/%{name}-%{version}-%{release}-root
And you missed the point that *in the context of Fedora Extras*, it's not broken.
-- Rex
On Wed, 2006-07-19 at 10:03 -0500, Rex Dieter wrote:
Ralf Corsepius wrote:
On Wed, 2006-07-19 at 16:15 +0200, Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
The point you seem to be missing, your buildroot is broken: buildroot: %{_tmppath}/%{name}-%{version}-%{release}-root
And you missed the point that *in the context of Fedora Extras*, it's not broken.
Rebuilding rpms by users is not amongst the Fedora tasks? FE is living in the ivory towers of mock?
With all due respect, ....
Ralf
Ralf Corsepius wrote :
On Wed, 2006-07-19 at 10:03 -0500, Rex Dieter wrote:
Ralf Corsepius wrote:
On Wed, 2006-07-19 at 16:15 +0200, Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
The point you seem to be missing, your buildroot is broken: buildroot: %{_tmppath}/%{name}-%{version}-%{release}-root
And you missed the point that *in the context of Fedora Extras*, it's not broken.
Rebuilding rpms by users is not amongst the Fedora tasks? FE is living in the ivory towers of mock?
With all due respect, ....
Oops. I missed this thread and started a very similar one, since reviewers are complaining about the same BuildRoot problem in my packages than in Axel's.
- I'd like non-broken BuildRoots to be considered OK (this should already be the case with "preferred" in the guidelines), and by non-broken I mean something like "starts with %{_tmppath}/ and contains at least a directory name unique to the package being built". Maybe the guidelines could be updated to include something like that.
- I'd like the FUD about appending `id -u` solving the problem for multi-user builds on the same system stop. Ralf, please : Users of a same system will also have conflicts with %_builddir, %_rpmdir and %_srcrpmdir by default and are *REQUIRED* to at least change their %_topdir to get things working. Well, just have them also change their %_tmppath as it can only be a positive thing.
Matthias
Le Mer 19 juillet 2006 17:03, Rex Dieter a écrit :
And you missed the point that *in the context of Fedora Extras*, it's not broken.
The Fedora packaging guidelines will also 1. be used by Fedora Core 2. serve as a model for countless packaging efforts in the wild so Fedora Extras context can not be assumed.
The buildroot chosen should not be simplified because of Extras buildsys specifics. It must be resilient to every rpm build scenario will know of, because as soon as Core uses it all these scenarii will be played out by someone somewhere. If anything, the OP message raeds the current buildroot is not safe enough yet.
Assumming no ISV will ever have several people build the same package on the same build server using plain Fedora buildroot is dangerous. Thinking they won't blame the Fedora platform when breakage happens is unrealistic.
Fedora is not just a 3rd party repo which can limit itself to whatever it chooses.
Regards,
On Wed, Jul 19, 2006 at 02:29:27PM +0200, Axel Thimm wrote:
Also consider what this really is about: Deambiguifying the BuildRoot per package makes sense as there may be several build processes sharing the same filesystem (either locally or through NFS), but deambiguifying the build user, too, means that we assume that the same EVR package will be possibly built on the same filesystem by different users? And even simultaneously?
On Wed, Jul 19, 2006 at 04:46:56PM +0200, Ralf Corsepius wrote:
On Wed, 2006-07-19 at 16:15 +0200, Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
The point you seem to be missing, your buildroot is broken: buildroot: %{_tmppath}/%{name}-%{version}-%{release}-root
No, I'm not, I added the original post above to prove I was aware of that ;)
This directory is NOT unique and will break if 2 or more users are running an rpmbuild in parallel on the same /var/tmp filesystem.
And everything will break if someone builds for i686 and i586 (e.g. a kernel or glibc) simultaneously on the same filesystem (as the same user), which is even worse and probably more common than two non-root users sharing the same build server and building *exactly* the same package EVR-wise.
It will also break if 2 different users are running buildjobs of the same package consecutively and if the first one fails and leaves it buildroot behind.
That's what rm -fr %{buildroot} at the beginning of %install is for. But even if it were an issue you are currently in the same more realistic situation that the build for the i686 kernel may fail and the next build is the one for i586 and will find the broken buildroot from the predecessor.
I'm just saying that we are focusing on an almost non-existant corner case obfuscating the BuildRoot while we allow failures in non-corner cases.
Axel.Thimm@ATrpms.net (Axel Thimm) writes:
This directory is NOT unique and will break if 2 or more users are running an rpmbuild in parallel on the same /var/tmp filesystem.
And everything will break if someone builds for i686 and i586 (e.g. a kernel or glibc) simultaneously on the same filesystem (as the same user), which is even worse and probably more common than two non-root users sharing the same build server and building *exactly* the same package EVR-wise.
ACK; when you build on multi-user systems, you should use a secure %_tmppath instead of trusting into %(id -u). Else, attacker could create between
| rm -rf $RPM_BUILD_ROOT | ... | make install --> mkinstalldir $RPM_BUILD_ROOT
an $RPM_BUILD_ROOT with e.g. files for symlink attacks (it should be trivial to find the window above with inotify(2)).
Therefore, multi-user environments are not an argument pro %(id -u).
Enrico
Le mercredi 19 juillet 2006 à 22:32 +0200, Enrico Scholz a écrit :
an $RPM_BUILD_ROOT with e.g. files for symlink attacks (it should be trivial to find the window above with inotify(2)).
Therefore, multi-user environments are not an argument pro %(id -u).
Yes it is. You are far more likely to share resources like a build system with friendlies than with attackers. So even if you don't protect against attackers, protecting against people stomping on each other is a worthwhile goal.
On Wed, 2006-07-19 at 21:02 +0200, Axel Thimm wrote:
I'm just saying that we are focusing on [...]
...generating a lengthy thread and a fuss about changing something that is not broken, thus negatively contributing to the already decreasing S/N ratio of packaging related discussions here and there? Sorry, can't help trolling, but that trend worries me.
On Wed, Jul 19, 2006 at 11:37:20PM +0300, Ville Skyttä wrote:
On Wed, 2006-07-19 at 21:02 +0200, Axel Thimm wrote:
I'm just saying that we are focusing on [...]
...generating a lengthy thread and a fuss about changing something that is not broken, thus negatively contributing to the already decreasing S/N ratio of packaging related discussions here and there? Sorry, can't help trolling, but that trend worries me.
I did say I assumed this to be an EasyFix item and was myself dissappointed that it turns out to become a lengthy thread. It takes more than one to generate a thread ...
And obviously this seems to be something many people have really strong opinion about and also different perception of whether this is broken or not. At this point of time I wished I wouldn't had bring it up, my opinion on that is not that strong and it doesn't seem to get anywhere.
On Wed, 2006-07-19 at 21:02 +0200, Axel Thimm wrote:
On Wed, Jul 19, 2006 at 02:29:27PM +0200, Axel Thimm wrote:
Also consider what this really is about: Deambiguifying the BuildRoot per package makes sense as there may be several build processes sharing the same filesystem (either locally or through NFS), but deambiguifying the build user, too, means that we assume that the same EVR package will be possibly built on the same filesystem by different users? And even simultaneously?
On Wed, Jul 19, 2006 at 04:46:56PM +0200, Ralf Corsepius wrote:
On Wed, 2006-07-19 at 16:15 +0200, Axel Thimm wrote:
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
The point you seem to be missing, your buildroot is broken: buildroot: %{_tmppath}/%{name}-%{version}-%{release}-root
No, I'm not, I added the original post above to prove I was aware of that ;)
This directory is NOT unique and will break if 2 or more users are running an rpmbuild in parallel on the same /var/tmp filesystem.
And everything will break if someone builds for i686 and i586 (e.g. a kernel or glibc) simultaneously on the same filesystem (as the same user),
True, i.e. defect/bug.
It will also break if 2 different users are running buildjobs of the same package consecutively and if the first one fails and leaves it buildroot behind.
That's what rm -fr %{buildroot} at the beginning of %install is for.
Nope, these buildroots will carry different uids => User2 cannot remove user1's buildroot.
Let me demonstrate it with one of your rpms:
# su -l user1 # rpmbuild -bi vtkdata.spec # exit
# su -l user2 # rpmbuild -bb vtkdata.spec ... + rm -rf /var/tmp/vtkdata-5.0.1-4-root rm: cannot remove `/var/tmp/vtkdata-5.0.1-4-root/usr/share/vtkdata-5.0.1/Data/cth.vtr': Permission denied rm: cannot remove `/var/tmp/vtkdata-5.0.1-4-root/usr/share/vtkdata-5.0.1/Data/Particles.raw': Permission denied ...
With FE's current buildroot this case doesn't happen.
I'm just saying that we are focusing on an almost non-existant corner case obfuscating the BuildRoot while we allow failures in non-corner cases.
I don't see how working in a multiuser environment would qualify as a corner case. Building for different target archs to me is a corner case, but that's probably a matter of personal background.
The above situation above would easily happen if working in a team, e.g. when developing an rpm.spec.
Ralf
On Wed, Jul 19, 2006 at 08:54:22AM -0500, Rex Dieter wrote:
Axel Thimm wrote:
Two independent reviewer considered this a blocker for a review's acceptance (even though it's marked "preferred").
The reviewers need to be whacked with a clue-stick. A working (non-broken) buildroot is *not* a blocker.
Care to lend me your clue-stick with instructions? I need to apply this to #220888? :)
packaging@lists.fedoraproject.org
-
Axel Thimm -
Enrico Scholz -
Jesse Keating -
Jesse Keating -
Matthias Saou -
Nicolas Mailhot -
Ralf Corsepius -
Rex Dieter -
Tom Callaway -
Ville Skyttä