build: remove x-prefix's from comparisons #23593
Conversation
|
Would it make sense to run one ci task with |
|
Concept ACK. |
|
Yes, seems about time. Concept ACK. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
|
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. |
|
Concept ACK |
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?
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. |
Yes, for basically all of our usage this "workaround" is pointless anyway.
I think the only case might be in some of the bdb detection logic.
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. |
|
Code review ACK 85b3656 |
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
fanquake
force-pushed
the
consolidate_remove_x_prefix_var_workaround
branch
from
85b3656 to
d6d402b
Compare
|
Concept ACK d6d402b |
|
Post merge ACK d6d402b, tested on netbsd |
9a5ce04 build: remove x-prefix comparisons (fanquake) Pull request description: 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/0fc4444fc0eea0ae88ac2f87434bf2c21f5b21da/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 ACKs for top commit: MarcoFalke: Concept ACK 9a5ce04 Tree-SHA512: 70030d61dcdb5b009823d83d73204627de53d2f628d8d6478e8e16804ac09f6bbdc53cbb1a6fb2085ebfe1a694b576e46ff381fb980cf667fab4bbadc79587d7
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
bitcoin/configure.ac
Line 292 in 681b25e
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