Bug 251110 - Mk/Scripts/qa.sh: stripped() can fail after pipefail was added; also misses the single file case
Summary: Mk/Scripts/qa.sh: stripped() can fail after pipefail was added; also misses t...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Port Management Team
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-13 18:23 UTC by John Hein
Modified: 2020-11-16 13:22 UTC (History)
4 users (show)

See Also:


Attachments
[patch] fix qa.sh:stripped() - possible pipefail + single file issues (v1) (577 bytes, patch)
2020-11-13 18:23 UTC, John Hein
jcfyecrayz: maintainer-approval? (portmgr)
Details | Diff
[patch] fix qa.sh:stripped() - possible pipefail + single file issues (v2) (587 bytes, patch)
2020-11-13 18:27 UTC, John Hein
jcfyecrayz: maintainer-approval? (portmgr)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Hein 2020-11-13 18:23:24 UTC
Created attachment 219641 [details]
[patch] fix qa.sh:stripped() - possible pipefail + single file issues (v1)

(1)
If there are any non-ELF files in the staging area, the stripped() test in qa.sh can fail now (silently) after 'set -o pipefail' was added.  This occurs if readelf from the binutils port is in your path ahead of readelf in base (tested on 11.x).  The former exits with a non-zero exit code if any input files are not ELF files.

$ /usr/bin/readelf -S /dev/null || echo FAILED
readelf: elf_begin() failed: Invalid argument
$ /usr/local/bin/readelf -S /dev/null || echo FAILED
readelf: Error: '/dev/null' is not an ordinary file
FAILED

We could try to only feed in ELF files (which may have a slight speed improvement because it avoids unecessary pipeline I/O or more likely might be slower because of an extra test).

Or we could tweak the pipeline to all ignore readelf errors.

We could also specify /usr/bin/readelf, which relies on readelf in base never changing course and returning non-zero exit code on errors.

Or we could do nothing because in most cases, /usr/bin/readelf will be found first.

Finally, we could temporarily disable pipefail for stripped() (causing all find + readelf errors to be ignored).


(2)
Note also that stripped() test fails to work as intended if there is only a single file because readelf -S does not print the 'File:' part unless there are multiple files (both versions of readelf behave the same here: base & binutils).  That is, if there is a single file and it is not stripped, the current implementation in stripped() misses it.  Easy fix: add /dev/null as a argument.


The attached patch specifies /usr/bin/readelf to fix (1) and adds /dev/null to fix (2) (and adds -- for safety in the very unlikely case any filenames start with -).
Comment 1 John Hein 2020-11-13 18:27:32 UTC
Created attachment 219642 [details]
[patch] fix qa.sh:stripped() - possible pipefail + single file issues (v2)

v2: This version of the patch ignores errors from readelf to fix (1).  This is safer in case base readelf is changed to return non-zero exit codes on errors (it probably should).

It uses the same fix for (2) as v1.
Comment 2 Jason W. Bacon freebsd_committer freebsd_triage 2020-11-14 14:22:37 UTC
The patch seems to work for me in both cases.
Comment 3 Po-Chuan Hsieh freebsd_committer freebsd_triage 2020-11-14 23:09:04 UTC
Assign to maintainer.
Comment 4 Li-Wen Hsu freebsd_committer freebsd_triage 2020-11-16 10:18:03 UTC
Was the attached patch or other fix committed?  I'm still getting this issue with r555443.
Comment 5 commit-hook freebsd_committer freebsd_triage 2020-11-16 10:51:50 UTC
A commit references this bug:

Author: mat
Date: Mon Nov 16 10:50:54 UTC 2020
New revision: 555463
URL: https://svnweb.freebsd.org/changeset/ports/555463

Log:
  Ignore readelf's return value.

  PR:		251110 (with style fixes)
  Submitted by:	John Hein

Changes:
  head/Mk/Scripts/qa.sh