Print binaries of non-matching arch in the warning/error message #4060
Conversation
This covers the post-classification logic in processBinaryFiles(). We will refactor this code in the next commits so add some tests. Use the 32-bit hello binary from the bundled i386 package and disable debuginfo when building with that, to avoid debuginfo related warnings on stderr. Also update some other tests that depend on the hello.spec "imprint" in some way as its contents is now changed.
|
Looking at this made me realize there's that other closely related check on the package arch that could use the same treatment: print the offending files. The existing "Binaries arch (%d) not matching the package arch (%d)" message with its abstract arch numbers is less than helpful and mostly just cryptic 😅 |
build/rpmfc.cc
Outdated
| } | ||
| rpmlog(terminate ? RPMLOG_ERR : RPMLOG_WARNING, | ||
| _("Arch dependent binaries in noarch package:\n%s"), | ||
| files.c_str()); |
There was a problem hiding this comment.
Hmm. Traditionally I think we've just done multiple rpmlog() calls in situations like this, which avoids the trouble of having to pre-formatting the string but then you get all those lines with a warning/error prefix. So we got
error: Arch dependent binaries in noarch package:
error: /usr/local/bin/hello
error: /usr/local/bin/hello2
vs
error: Arch dependent binaries in noarch package:
/usr/local/bin/hello
/usr/local/bin/hello2
I originally intended to say the filenames prefixed with error/warning doesn't look horrible (to me), but seeing these side by side, it actually does 😆 So the bit of extra trouble to preformat the string does seem worth it. You've obviously pondered this already, this is just me talking myself through it. Feel free to ignore this particular blahblah 😅
There was a problem hiding this comment.
Indeed 😅 But, to be honest, I don't mind the "error:" variant either. It'd simplify the code a bit and we wouldn't need to worry about indenting etc. It would also make the message stand out better - these are all basically error lines in the output.
One could say every log message should be meaningful on its own (which these wouldn't) but then, it doesn't really matter as we always print them together anyway.
Then there's the precedent of the "unpackaged files" error which also dumps them in one log entry, but... I kinda like the prefixed variant more now 😄 We'll see. I'll throw a coin, I guess.
|
So .. I could hit merge, but maybe the other check is worth doing in the same PR? |
Yep, based on our chat I'll generalize this for both checks. Turning to draft for that. |
Extract the per-package color checks from processBinaryFiles() into a static function in rpmfc and call it from rpmfcGenerateDepends(). This makes a bit more sense because RPMTAG_HEADERCOLOR is derived from RPMTAG_FILECOLORS, which in turn is determined at file classification time in rpmfcClassify(). In particular, this will enable us to provide per-file info in the error message in case some of these checks fail. This moves the error output to before the dependencies are printed, but that's fine since there's the "RPM build errrors" summary anyways. The logic itself, though, is executed in pretty much the same place as before. This also means the checks no longer execute for packages with no files but that, again, makes more sense. Related: rpm-software-management#3600
Make it easier to locate the offending files when there are multiple subpackages being built. Move the rpmlog() call out of the conditionals so that we can (de)allocate the nvr more easily. This also prepares the ground for the next commit. Rephrase the "Binaries not matching" warning and drop the numerical representations of the header/arch colors as those aren't exactly meaningful to the user, oops.
|
Generalized the file listing and added shortened file types to the printed filenames. Some of the commits could perhaps be squashed together but I figured separating them would make it easier to review 😄 |
dmnks
changed the title
When we detect that some files (based on RPMTAG_HEADERCOLOR) don't match the package's arch color, print them to the build log as well, to make troubleshooting easier. Keep the existing check using RPMTAG_HEADERCOLOR as a simple heuristic that allows us to bail out immediately in case there's nothing to print. Indent the file list by 4 spaces to make it easier to visually parse and to align it with the "RPM build errors" summary. This would ideally be done by the rpmlog routines (when dealing with multiline messages) but we don't have that (yet) so just prepend each line with spaces manually. Only show the installed path (without the buildroot prefix) for brevity. Fixes: rpm-software-management#3600
Invert and split the conditionals to make the whole (hopefully) a bit easier to read. No functional change.
When listing the offending files, also show the file type, it may be handy. Trim it after the first space char but keep at least 10 chars, that should suffice as a hint to the user, and conveniently covers the ELF binary types (see the updated test).
2 participants
Include the actual binaries with the rpmbuild error message "Arch dependent binaries in noarch package" when it happens, for example:
Fixes: #3600