Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

%exclude should not permit files to bypass check-files and be omitted from all packages built from spec #994

Open
Conan-Kudo opened this issue Jan 2, 2020 · 22 comments · Fixed by #1442
Labels

Comments

@Conan-Kudo
Copy link
Member

Conan-Kudo commented Jan 2, 2020

As part of some of the work I've done in OpenMandriva in transitioning the RPM stack from rpm5.org to rpm.org RPM, I've discovered that there was an interesting behavioral difference with %exclude.

In rpm5.org RPM, %exclude does not give you a "get out of jail free" card to bypass the file list check. A file that is marked by %exclude in one subpackage but isn't included in any other subpackage triggers the unpackaged files error. This does not happen in rpm.org RPM.

I would argue that the rpm.org behavior is a bug, as there's not a particularly obvious reason for why it works this way. Moreover, it leads to accidental packaging bugs.

Can we change this behavior for the upcoming RPM 4.16?

@Conan-Kudo
Copy link
Member Author

(as for why I'm filing this now... well, I forgot about this in the shuffle two years ago, and I was just reminded of this again today...)

@pmatilai
Copy link
Member

pmatilai commented Jan 2, 2020

Sounds like a bug indeed. Patches welcome ;)

@ignatenkobrain
Copy link
Contributor

I am predicting this will break multiple packages in Fedora, but I think this would be good behavior.

@voxik
Copy link
Contributor

voxik commented Jan 3, 2020

It will definitely break something, probably a lot of things. OTOH, it would probably make the things more consistent, e.g. it would avoid issues such as [1], because one would need to delete the file instead of excluding it.

@hroncok
Copy link
Contributor

hroncok commented Jan 3, 2020

This will break a lot of packages in Fedora indeed.

For some of my packages I even rely on the behavior, such as here: https://src.fedoraproject.org/rpms/python-xmlschema/blob/a11ca456b41794af7aec847af66f1e7974a698ff/f/python-xmlschema.spec#_59

@kkofler
Copy link

kkofler commented Jan 21, 2020

The current behavior of %exclude is a feature and should not be incompatibly changed.

@pmatilai
Copy link
Member

The spirit of %exclude always was merely to support sub-packaging, not for leaving random junk in buildroot. But where even the tiniest crack exists, just like water will find its way, so will packagers 😄

@Conan-Kudo
Copy link
Member Author

Conan-Kudo commented Jan 21, 2020

My current guess on how this slips through is that files that are marked as excluded are still being passed to check-files, so it passes the check-files check. But my attempts at trying to make it not do that seem to be in vain... 😕

I at least have a test case to see if I fixed it, but so far, no dice. 😢

@pmatilai pmatilai added the bug label Feb 24, 2020
@ffesti ffesti added this to Needs triage in Ticket Review (Outdated) Mar 3, 2020
@pmatilai pmatilai changed the title RFE: %exclude should not permit files to bypass check-files and be omitted from all packages built from spec %exclude should not permit files to bypass check-files and be omitted from all packages built from spec Mar 30, 2020
@ffesti ffesti moved this from Needs triage to Yes in Ticket Review (Outdated) Mar 30, 2020
pmatilai added a commit to pmatilai/rpm that referenced this issue Nov 18, 2020
…nt#994)

The intent of %exclude was always to merely support sub-packaging with
wildcards in %files sections, not to permit leaving junk in the buildroot.
Enforce this by checking against the actually packaged contents rather
than everything we encountered during collection. This also gets rid
of the ugly static check_fileList buffer - besides being ugly, such things
are bad for parallelism.

This has been widely abused so the change is likely to break quite a few
packages in the wild. As a side-effect this also cures a long-standing bug
where unpackaged excluded files leak their debuginfo into packaged contents,
as such a package will now fail to build at (RhBug:878863)

Fixes: rpm-software-management#994
pmatilai added a commit to pmatilai/rpm that referenced this issue Nov 18, 2020
…nt#994)

The intent of %exclude was always to merely support sub-packaging with
wildcards in %files sections, not to permit leaving junk in the buildroot.
Enforce this by checking against the actually packaged contents rather
than everything we encountered during collection, and document the
behavior.

This has been widely abused so the change is likely to break quite a few
packages in the wild. As a side-effect this also cures a long-standing bug
where unpackaged excluded files leak their debuginfo into packaged contents,
as such a package will now fail to build at (RhBug:878863)

As a nice side-bonus, this also gets rid of the ugly static check_fileList
buffer - besides being ugly, such things are bad for parallelism.

Fixes: rpm-software-management#994
Ticket Review (Outdated) automation moved this from Yes to Closed Nov 19, 2020
pmatilai added a commit that referenced this issue Nov 19, 2020
The intent of %exclude was always to merely support sub-packaging with
wildcards in %files sections, not to permit leaving junk in the buildroot.
Enforce this by checking against the actually packaged contents rather
than everything we encountered during collection, and document the
behavior.

This has been widely abused so the change is likely to break quite a few
packages in the wild. As a side-effect this also cures a long-standing bug
where unpackaged excluded files leak their debuginfo into packaged contents,
as such a package will now fail to build at (RhBug:878863)

As a nice side-bonus, this also gets rid of the ugly static check_fileList
buffer - besides being ugly, such things are bad for parallelism.

Fixes: #994
@kkofler
Copy link

kkofler commented Nov 19, 2020

Again: how is this an improvement? I have seen many specfiles deliberately using %exclude in the way that you are now prohibiting. This is an incompatible change making packaging unnecessarily harder. And Miro @hroncok even posted a case where the obvious fix (using rm instead) won't work because the files are needed in %check.

@Conan-Kudo
Copy link
Member Author

@kkofler Just because @hroncok is doing that does not mean it was a good idea to do it that way to begin with. Additionally, that would have been broken anyway if you tried to %exclude a binary file that had associated debug symbol files, since it would wind up generating a dangling debuginfo symbol file. If we want a stanza that lets you filter out files from all subpackages, we need a different way to do it anyway.

@pmatilai
Copy link
Member

pmatilai commented Nov 19, 2020

Yes, I too have seen an endless stream of specfiles deliberately doing all manner of strange things and abusing loopholes in rpmbuild, and we've been systematically closing those loopholes as we come across them and time permits, for (more) consistent and defined behavior. Just like compilers do. This is just another in a long string of such changes - buildroot always was only for packaged material.

@kkofler
Copy link

kkofler commented Nov 19, 2020

Just like compilers do.

I am also complaining just the same way about compilers doing this. Your "closing loopholes" is your users' incompatible changes that unnecessarily break their builds.

@pmatilai
Copy link
Member

If it was unnecessary, I'd agree...

@kkofler
Copy link

kkofler commented Nov 19, 2020

I think your definition of "necessary" differs significantly from mine. RPM will not break down if this incompatible change is not made (or reverted, now that you pushed it), so I do not see why it is necessary.

And to give some context: as a maintainer of TIGCC, I was consistently reverting that kind of changes to our patched GCC (C only though, because we did not support C++ (whose GCC frontend is much worse in this respect) for other, unrelated reasons). I did all I could to maintain backwards source compatibility all the way to the initial alpha release of TIGCC and I strongly believe that this should be the standard to which to hold all compilers. E.g., the TIGCC patch reenables multiline C string literals, C casts as lvalues, etc. So I am not just a passive complainer.

@pmatilai
Copy link
Member

RPM may not immediately break down if one such change is not done, but maintaining bug compatibility for bug compatibility's sake is a colossal waste of time, and worse, they sooner or later end up preventing new developments from taking place. That's why maintaining those undocumented dark corner behaviors that somebody found is bad for everybody.

@hroncok
Copy link
Contributor

hroncok commented Nov 19, 2020

I think there is no point in arguing. I understand both sides. Let's try measure the impact of this? Maybe it's not horrible, only pretty bad :)

pmatilai added a commit to pmatilai/rpm that referenced this issue Nov 19, 2020
Previously we only checked for unpackaged files and symlinks, completely
ignoring eg extra directories that might be there. Just check for everything
instead. Related to rpm-software-management#994.

Directories are a little tricky as some of them are almost always unowned
so we need to allow all path components leading to packaged files.
pmatilai added a commit to pmatilai/rpm that referenced this issue Nov 20, 2020
Previously we only checked for unpackaged files and symlinks, completely
ignoring eg extra directories that might be there. Just check for everything
instead. Related to rpm-software-management#994.

Directories are a little tricky as some of them are almost always unowned
so we need to allow all path components leading to packaged files. This
would be a *lot* of relatively expensive lookups, but we only need to do
the dance when the directory index changes, and we can stop when no new
paths get added to the pool.
@hroncok
Copy link
Contributor

hroncok commented Nov 21, 2020

Maybe it's not horrible, only pretty bad :)

It is horrible. See #1442 (comment)

pmatilai added a commit that referenced this issue Nov 23, 2020
Previously we only checked for unpackaged files and symlinks, completely
ignoring eg extra directories that might be there. Just check for everything
instead. Related to #994.

Directories are a little tricky as some of them are almost always unowned
so we need to allow all path components leading to packaged files. This
would be a *lot* of relatively expensive lookups, but we only need to do
the dance when the directory index changes, and we can stop when no new
paths get added to the pool.
@pmatilai
Copy link
Member

pmatilai commented Apr 8, 2021

The change got reverted for now, reopening. Sigh.

@pmatilai pmatilai reopened this Apr 8, 2021
Ticket Review (Outdated) automation moved this from Closed to Needs triage Apr 8, 2021
@Conan-Kudo
Copy link
Member Author

😭

@Conan-Kudo
Copy link
Member Author

@pmatilai Could we have an (extra) knob for this behavior and have Fedora switch it off by default? I think of the main distributions using/contributing to RPM, only Fedora does not expect to enforce package file list consistency because it runs no package build verification tools as automatic post-build checks that can fail the build.

@hroncok
Copy link
Contributor

hroncok commented Apr 8, 2021

Ideas for progress:

  • Open a ticket at Fedora Packaging Committee or better send a PR to File and Directory Ownership (source) explaining why %exluding files completely from packages is dangerous and not intended to work and that it MUST not be done.
  • Work with @voxik to change the rubygem package generator.
  • Work with me to solve the Python namespace package issue. For example in this bugzilla. Maybe %ghosting is a way to go.
  • Open a ticket on rpmlint to detect completely %exluded files. Not sure if it is technically possible, but worth a shot.
  • Open a ticket on FedoraReview to detect completely %exluded files. Should be possible.
  • In %files, collect the list of %exluded files and see if all of them are actually packages somewhere. If not, issue a warning that could be knob-ed to an error.

Note that those ideas are not dependent on each other and can happen at different timelines.

@pmatilai
Copy link
Member

pmatilai commented Apr 8, 2021

The reason for reverting is that there's this unexpected link to %check usage (and ambiguity) and I think those changes are better handled together. I have zero more cycles and/or will to spare on this topic right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

6 participants