Description of problem: on F28 and F29 build of uucp create a wrong Requirement /usr/share/uucp/contrib/xchat , that does not on uucp-1.07-51.fc27 reading build.log [1] we find Requires: /bin/sh /usr/bin/perl /usr/share/uucp/contrib/xchat libc.so.6()(64bit) libc.so.6(GLIBC_2.14)(64bit) libc.so.6(GLIBC_2.15)(64bit) libc.so.6(GLIBC_2.2.5)(64bit) libc.so.6(GLIBC_2.27)(64bit) libc.so.6(GLIBC_2.3)(64bit) libc.so.6(GLIBC_2.3.4)(64bit) libc.so.6(GLIBC_2.4)(64bit) libc.so.6(GLIBC_2.7)(64bit) liblockdev.so.1()(64bit) rtld(GNU_HASH) [1] https://kojipkgs.fedoraproject.org//packages/uucp/1.07/53.fc28/data/logs/x86_64/build.log Version-Release number of selected component (if applicable): builds of uucp-1.07-53.fc28 and uucp-1.07-54.fc29
$ head -n1 usr/share/uucp/contrib/Dial.Hayes #! /usr/share/uucp/contrib/xchat Not a bug in RPM.
The longer story here is that in Fedora >= 28, there's a new buildroot policy (brp-mangle-shebangs) which does some sanity checks, transformations and whitespace cleanup of shebang lines. There actually is an rpm bug here but is basically reverse of what you think: rpm's dependency generator doesn't pick up shebang lines when there's whitespace between the shebang and path. Now that rpm-mangle-shebangs removes the whitespace, rpm's depgen sees the shebang and generates a dependency for it. So what you get in Fedora >= 28 is correct, older versions are buggy. To fix, either change the shebangs to a legitimate ones or chmod a-x the files in contrib/, by a quick look most of them certainly should not be executable anyway. The perl-requires thing (bug 1539344) is entirely unrelated.
Switching against redhat-rpm-config. Please ensure we mangle only necessary set of scripts, and only those where we are 100% sure what we are doing. Please also add test-suite so we test that the shebang mangler behaves sanely. I'll drop the executable bit from those scripts, but really -- we shouldn't generate additional requirements this way. That just pollutes fixed dep graph. IOW, packager should be responsible for specifying the set of language runtimes his package needs.. not the fact that package install some innocent example (but still) executable script.
(In reply to Pavel Raiskup from comment #3) > Switching against redhat-rpm-config. Please ensure we mangle only > necessary set of scripts, and only those where we are 100% sure what > we are doing. Please also add test-suite so we test that the > shebang mangler behaves sanely. > > I'll drop the executable bit from those scripts, but really -- we > shouldn't generate additional requirements this way. That just > pollutes fixed dep graph. IOW, packager should be responsible for > specifying the set of language runtimes his package needs.. not > the fact that package install some innocent example (but still) > executable script. Hey, logs don't show that we change those shebangs (and we actually do log all changes). It's uucp which has executable files with wrong shebangs.
(In reply to Igor Gnatenko from comment #4) > Hey, logs don't show that we change those shebangs (and we actually do log > all changes). It's uucp which has executable files with wrong shebangs. You are right (it is result of some invalid patch which was aimed to fix the mangling policy), but still please see the dep-hell point. Having installed /usr/share/<PKG>/contrib/examples/foo executable script with /usr/bin/bf shebang doesn't really mean I want to install brainfuck unconditionally everywhere.
(In reply to Pavel Raiskup from comment #5) > (In reply to Igor Gnatenko from comment #4) > > Hey, logs don't show that we change those shebangs (and we actually do log > > all changes). It's uucp which has executable files with wrong shebangs. > > You are right (it is result of some invalid patch which was aimed to > fix the mangling policy), but still please see the dep-hell point. > > Having installed /usr/share/<PKG>/contrib/examples/foo executable > script with /usr/bin/bf shebang doesn't really mean I want to install > brainfuck unconditionally everywhere. This is the behavior RPM had for last... since ever? Recently Panu changed to not generate dependencies on %doc files. So you probably want to use %doc for those files.
> This is the behavior RPM had for last... since ever? Can we consider this to be a RFE, then? Btw. I dropped executable bits from all contrib files for now in uucp. > Recently Panu changed to not generate dependencies on %doc files. So you > probably want to use %doc for those files. It is valid, thanks, but seems to be incomplete. I doubt it makes sense to analyse anything else than some set of privileged directories (%_bindir, %_libexecdir, etc.).
The executable bit has always been *the* deciding factor whether automatic dependency generation is considered for a file or not, and that's not going to change. It wasn't me who added %doc filtering, in fact I tend to disagree with that, but that's another story. Filtering by location is almost always wrong - if it's marked executable then in order to execute it, the shebang interpreter IS required. And if it's not supposed to be executed it shouldn't have the executable bit set, period, no matter where it is. Finally, I said that this was due to brp-mangle-shebangs trimming out whitespace, but turns out that's not the case. brp-mangle-shebang changes are logged, and rpm does actually generate dependencies if there's whitespace. So wth does the difference between F27 and F28 come from? This change in uucp: https://src.fedoraproject.org/rpms/uucp/c/aeeedcfda2ab131607997288f41df4f782a34d82?branch=master Back to uucp, and please don't reassign back because this behavior in rpm is not going to change.
(In reply to Panu Matilainen from comment #8) > The executable bit has always been *the* deciding factor whether automatic > dependency generation is considered for a file or not, and that's not going > to change. Why? Please reconsider. I'll submit a new issue for this. > It wasn't me who added %doc filtering, in fact I tend to disagree with that, > but that's another story. Filtering by location is almost always wrong - if > it's marked executable then in order to execute it, the shebang interpreter > IS required. And if it's not supposed to be executed it shouldn't have the > executable bit set, period, no matter where it is. I don't really understand why we pollute the requirements by innocent files under innocent directories. Dropping executable bit or making some some (example, or helper) scripts to be "just" documentation files is way too much. And adding it's shebang to Requires: is way too much, too. But it might be corner case, nevermind ... > Finally, I said that this was due to brp-mangle-shebangs trimming out > whitespace, but turns out that's not the case. brp-mangle-shebang changes > are logged, and rpm does actually generate dependencies if there's > whitespace. So wth does the difference between F27 and F28 come from? This > change in uucp: > > https://src.fedoraproject.org/rpms/uucp/c/ > aeeedcfda2ab131607997288f41df4f782a34d82?branch=master Yes.
(In reply to Pavel Raiskup from comment #9) > (In reply to Panu Matilainen from comment #8) > > The executable bit has always been *the* deciding factor whether automatic > > dependency generation is considered for a file or not, and that's not going > > to change. > > Why? Please reconsider. I'll submit a new issue for this. No. Please dont. > > > It wasn't me who added %doc filtering, in fact I tend to disagree with that, > > but that's another story. Filtering by location is almost always wrong - if > > it's marked executable then in order to execute it, the shebang interpreter > > IS required. And if it's not supposed to be executed it shouldn't have the > > executable bit set, period, no matter where it is. > > I don't really understand why we pollute the requirements by innocent > files under innocent directories. Dropping executable bit or making some > some (example, or helper) scripts to be "just" documentation files is way > too much. And adding it's shebang to Requires: is way too much, too. > But it might be corner case, nevermind ... > WTF is "innocent directory"? See my rationale above, if it's not supposed to be executed then it bloody well better not be marked executable. End of story.
(In reply to Panu Matilainen from comment #10) > WTF is "innocent directory"? See my rationale above, if it's not supposed to > be executed then it bloody well better not be marked executable. End of > story. E.g. contrib directory in case of uucp. The fact that I put there something executable doesn't mean I want to install the deps for it. So I have to make it non-executable, or I have to mark it %doc. It's equivalent to weak-deps where we also accept the fact that package has some built-in functionality which is not usable unless you install some weak dep.
There's an RFE to turn executable %doc file dependencies into weak dependencies in bug 1476594, and that I think is a rather reasonable compromise to this. If the contrib/ is as unused old cruft as it seems then maybe it a) should be doc (so it can excluded with --nodocs) b) should be split into a separate subpackage (which is the way to make things optional, not leaving executable files lying around) c) not packaged at all
Agreed, plus: d) turn off the shebang analyzer to not pollute the dep-graph Please switch from concrete example to generic POV, and then the d) might be the best option semantically... Since such scripts might be useful to someone (executable bit needed), thus installed by default (but in container world we use --nodocs by default), and still it is "just" optional functionality to add any dependency. The drawback of d) is that I have to put shell and maybe other lang into Requires:, but seems to be worth it. Back to the example -- in this case, I dropped the executable bits, which is not really correct. If anyone feels like we need better fix, please reopen or fix directly.
(In reply to Pavel Raiskup from comment #13) > Back to the example -- in this case, I dropped the executable bits, > which is not really correct. I think the best solution is to put the contrib examples into a separate package. The contrib examples should work out of the box, so should be executable and have the dependencies that they need. But they don't need to be in the main package. That way you can easily make them optional for most users.
Exactly - the way to ship "optional" scripts is to put them in subpackage(s). If they scripts are to be of use to anybody they need to have the script interpreter installed anyway, hence the dependencies need to be there. Otherwise you're simply shipping a buggy package and somebody will file a bug "unable to execute this thing here". And IMO that's all there is to it. In this case the shebang was totally bogus so it wasn't possible to actually execute those files to begin with. So really the reasonable options are to either fix the shebang so they CAN be executed, or turn off executable bit to set expectations. Or stop packaging the cruft.
Mark, it is IMO not worth the subpackage. One would have to properly document that the subpackage actually exists; otherwise it is equivalent to removal. If you feel some of those scripts deserve executable bit (and thus additional Requires fields), let me know. Otherwise I'll let more optimizations to other maintainers, because I feel a huge discomfort to fight with over-automation. Really, I would much rather take the responsibility to specify the Requires explicitly for these types of scripts. I would open a bug, but let's accept the wish above.