depends: Allow PATH with spaces in directory names. #28733
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
Github-Pull: bitcoin#28733 Rebased-From: 92f7e7f
| # This is needed to find uname on a Pyramid OSx when run in the BSD universe. | ||
| # ([email protected] 1994-08-24) | ||
| if test -f /.attbin/uname ; then | ||
| PATH=$PATH:/.attbin ; export PATH |
There was a problem hiding this comment.
Is this actually needed? Seems like a completely arcane check and I'm not sure how this would be tokenized wrong.
There was a problem hiding this comment.
This is also an upstream file, so shouldn't be modified here. If we do want to change it, we should first pull the latest upstream version, and also send a patch upstream.
There was a problem hiding this comment.
It's been updated as recently as April of this year. Getting it fixed upstream would be ideal, but surely that shouldn't get in the way of fixing the issue locally?
There was a problem hiding this comment.
Is this actually fixing anything though?
There was a problem hiding this comment.
i would be surprised if this code path is ever reached, apparently Pyramid OSx is a discontinued operating system from the 90's. Nothing that runs that will run bitcoin core even if we cared supporting it 😄
fanquake
changed the title
|
Concept ACK |
|
Concept ACK. I guess longer term it would be good to have the CI run in a path that has a space and some emojis in it. |
|
Concept ACK |
49a9257 build: latest config.sub in depends (fanquake) ced0435 build: latest config.guess in depends (fanquake) Pull request description: Before we make any local modifications (i.e #28733) pull the latest files from upstream. ACKs for top commit: TheCharlatan: ACK 49a9257 Tree-SHA512: fbbe0d6ef72a196a652467af0550b38da23b932fe68da4965a9b0dc4795db9c869969db98f660cd360f6af3a7659b46c25e3fd398e0ef127dae71726b9a915a6
|
Are you still working on this? If yes, it would be good to address the outstanding feedback. Also, it would be good to add a test for this, to avoid breaking it in the future. For example: diff --git a/ci/test/02_run_container.sh b/ci/test/02_run_container.sh
index edf8f2c30..f82bbcd23 100755
--- a/ci/test/02_run_container.sh
+++ b/ci/test/02_run_container.sh
@@ -70,7 +70,7 @@ if [ "$CI_OS_NAME" == "macos" ]; then
fi
CI_EXEC () {
- $CI_EXEC_CMD_PREFIX bash -c "export PATH=${BINS_SCRATCH_DIR}:${BASE_ROOT_DIR}/ci/retry:\$PATH && cd \"${BASE_ROOT_DIR}\" && $*"
+ $CI_EXEC_CMD_PREFIX bash -c "export PATH=\"/path_with space:${BINS_SCRATCH_DIR}:${BASE_ROOT_DIR}/ci/retry:\$PATH\" && cd \"${BASE_ROOT_DIR}\" && $*"
}
export -f CI_EXEC
As I understand, this doesn't allow spaces in PATH in any relevant part used by Bitcoin Core, only a space in unused parts of the PATH. |
|
Marked as up-for-grabs. Should be trivial to fixup and bring over the finish line. |
4756114 [depends] Allow PATH with spaces in directory names. (Mark Friedenbach) Pull request description: The goal of this PR is to help close #28733. I reverted the change on `depends/config.guess` based on the feedback provided in the previous PR. I've also incorporated the test mentioned by maflcko ACKs for top commit: maflcko: lgtm ACK 4756114 hebasto: ACK 4756114, successfully built depends on Ubuntu 22.04. TheCharlatan: ACK 4756114 Tree-SHA512: ee257f6efd235839156bc236384f08d77b91debc3c257168368a71e70742639f28a3289572b8693609c1109062dc9968e461103d1f4f5679906506e94b54e649
49a9257 build: latest config.sub in depends (fanquake) ced0435 build: latest config.guess in depends (fanquake) Pull request description: Before we make any local modifications (i.e bitcoin#28733) pull the latest files from upstream. ACKs for top commit: TheCharlatan: ACK 49a9257 Tree-SHA512: fbbe0d6ef72a196a652467af0550b38da23b932fe68da4965a9b0dc4795db9c869969db98f660cd360f6af3a7659b46c25e3fd398e0ef127dae71726b9a915a6
4756114 [depends] Allow PATH with spaces in directory names. (Mark Friedenbach) Pull request description: The goal of this PR is to help close bitcoin#28733. I reverted the change on `depends/config.guess` based on the feedback provided in the previous PR. I've also incorporated the test mentioned by maflcko ACKs for top commit: maflcko: lgtm ACK 4756114 hebasto: ACK 4756114, successfully built depends on Ubuntu 22.04. TheCharlatan: ACK 4756114 Tree-SHA512: ee257f6efd235839156bc236384f08d77b91debc3c257168368a71e70742639f28a3289572b8693609c1109062dc9968e461103d1f4f5679906506e94b54e649
4756114 [depends] Allow PATH with spaces in directory names. (Mark Friedenbach) Pull request description: The goal of this PR is to help close bitcoin#28733. I reverted the change on `depends/config.guess` based on the feedback provided in the previous PR. I've also incorporated the test mentioned by maflcko ACKs for top commit: maflcko: lgtm ACK 4756114 hebasto: ACK 4756114, successfully built depends on Ubuntu 22.04. TheCharlatan: ACK 4756114 Tree-SHA512: ee257f6efd235839156bc236384f08d77b91debc3c257168368a71e70742639f28a3289572b8693609c1109062dc9968e461103d1f4f5679906506e94b54e649
8 participants
Title pretty much says it all. On one of my systems there is a component of the PATH which has a space in it, so expanding
$PATHwill be tokenized by the shell. This PR just adds a few key quotes to make sure that expansions of$PATHare handled correctly.