build: remove x-prefix’s from comparisons #23593

1. 
   fanquake commented at 1:21 pm on November 25, 2021: member

   Very old shells suffered from bugs which meant that prefixing variables with an “x” to ensure that the lefthand side of a comparison always started with an alphanumeric character was needed. Modern shells don’t suffer from this issue (i.e Bash was fixed in 1996).

   In any case, we’ve already got unprefixed checks used in our codebase, i.e https://github.com/bitcoin/bitcoin/blob/681b25e3cd7d084f642693152322ed9a40f33ba0/configure.ac#L292 and have libs (in depends) that also use unprefixed comparisons in their configure scripts.

   I think it’s time that we consolidate on not using the x-prefix workaround. At best it’s mostly just confusing. Could simplify some of these checks further in future.

   More info: https://github.com/koalaman/shellcheck/wiki/SC2268 https://www.vidarholen.net/contents/blog/?p=1035

2. fanquake added the label Build system on Nov 25, 2021
3. 
   promag commented at 1:38 pm on November 25, 2021: member

   Concept ACK.
4. 
   MarcoFalke commented at 1:59 pm on November 25, 2021: member

   Would it make sense to run one ci task with CONFIG_SHELL zsh?
5. 
   hebasto commented at 2:07 pm on November 25, 2021: member

   Concept ACK.
6. 
   laanwj commented at 2:14 pm on November 25, 2021: member

   Yes, seems about time. Concept ACK.
7. 
   hebasto commented at 4:33 pm on November 25, 2021: member

   Another link: https://www.gnu.org/software/automake/faq/autotools-faq.html#What-does-_0027test-_0022X_0024variable_0022-_003d-X_0027-mean_003f
8. 
   DrahtBot commented at 8:05 am on November 26, 2021: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #23675 (build: Fix build for i686-linux-android host and some small cleanups after pr23489 by hebasto)
   • #23609 (build: Enables reduced exports by default for macOS host by hebasto)
   • #22708 (build, qt: Add Wayland support for Linux builds with depends by hebasto)
   • #20201 (build: pkg-config related cleanup by hebasto)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

9. 
   theStack commented at 3:45 pm on November 27, 2021: member

   Concept ACK
10. 
    laanwj commented at 11:52 am on November 29, 2021: member

    Tested successfully on FreeBSD 13.0. Would be good if someone tested on OpenBSD, NetBSD? (I might, but don’t have an up-to-date install at the moment). I expect the more obscure BSDs to be the largest risk of not supporting this.
11. 
    theStack commented at 12:45 pm on November 29, 2021: member

    Tested on OpenBSD 7.0, everything works as expected. (fresh build, configured with --without-gui --without-bdb --disable-external-signer).
12. 
    gruve-p commented at 5:47 pm on November 30, 2021: contributor

    Concept ACK
13. 
    luke-jr commented at 10:33 pm on November 30, 2021: member

    In any case, we’ve already got unprefixed checks used in our codebase, i.e

    If it’s about non-alphanumeric characters, clearly it isn’t needed for yes/no values - do we have any cases where we might get passed a path?

    and have libs (in depends) that also use unprefixed comparisons in their configure scripts.

    Builds don’t have to use depends, and presumably any platform affected would have solved this for their packages already.

    That being said… we require C++17 now. If only ancient shells have this problem, seems fine.

14. 
    fanquake commented at 5:25 am on December 1, 2021: member

    If it’s about non-alphanumeric characters, clearly it isn’t needed for yes/no values

    Yes, for basically all of our usage this “workaround” is pointless anyway.

    do we have any cases where we might get passed a path?

    I think the only case might be in some of the bdb detection logic.

    Builds don’t have to use depends,

    Of course not. However in general, I’d guess that people building on older systems would be more likely to use depends, because it’s a convenient way to get access to newer versions of our required dependencies (when system packages aren’t being updated), and thus we’d have been more likely to see it reported if it was actually an issue.

15. 
    laanwj commented at 3:20 pm on December 2, 2021: member

    Code review ACK 85b3656a328eb77546deb8533d10e411a0b1872d
16. build: remove x-prefix comparisons

    Very old shells suffered from bugs which meant that prefixing variables
    with an "x" to ensure that the lefthand side of a comparison always
    started with an alphanumeric character was needed. Modern shells don't
    suffer from this issue (i.e Bash was fixed in 1996).
    
    In any case, we've already got unprefixed checks used in our codebase,
    i.e https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L292,
    and have dependencies (in depends) that also use unprefixed comparisons.
    
    I think it's time that we can consolidate on not using the x-prefix
    workaround. At best it's mostly just confusing.
    
    More info:
    https://github.com/koalaman/shellcheck/wiki/SC2268
    https://www.vidarholen.net/contents/blog/?p=1035

    d6d402bd2b
17. fanquake force-pushed on Dec 3, 2021
18. 
    MarcoFalke commented at 9:09 am on December 7, 2021: member

    Concept ACK d6d402bd2b8211e9e111acae433ca4e3cfd8d370
19. MarcoFalke merged this on Dec 7, 2021
20. MarcoFalke closed this on Dec 7, 2021

    ---

21. sidhujag referenced this in commit dc3eed3732 on Dec 7, 2021
22. 
    jarolrod commented at 0:24 am on December 8, 2021: member

    Post merge ACK d6d402b, tested on netbsd
23. RandyMcMillan referenced this in commit d8e9b178d6 on Dec 23, 2021
24. fanquake deleted the branch on May 31, 2022
25. DrahtBot locked this on May 31, 2023

Contributors
fanquake promag MarcoFalke hebasto laanwj DrahtBot theStack gruve-p luke-jr jarolrod

Labels
Build system