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

Conan-Kudo commented 4 years ago

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 commented 4 years ago

(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 commented 4 years ago

Sounds like a bug indeed. Patches welcome ;)

ignatenkobrain commented 4 years ago

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

voxik commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

pmatilai commented 4 years ago

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 :smile:

Conan-Kudo commented 4 years ago

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. 😢

kkofler commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

kkofler commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 :)

hroncok commented 3 years ago

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

It is horrible. See https://github.com/rpm-software-management/rpm/pull/1442#issuecomment-731554917

pmatilai commented 3 years ago

The change got reverted for now, reopening. Sigh.

Conan-Kudo commented 3 years ago

😭

Conan-Kudo commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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.

rpm-software-management / rpm

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