depends: Allow PATH with spaces in directory names. #28733

1. 
   maaku commented at 9:44 pm on October 25, 2023: contributor

   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 $PATH will be tokenized by the shell. This PR just adds a few key quotes to make sure that expansions of $PATH are handled correctly.
2. [depends] Allow PATH with spaces in directory names. 92f7e7f363
3. 
   DrahtBot commented at 9:44 pm on October 25, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	laanwj, maflcko, kristapsk

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. luke-jr referenced this in commit fed7cb557a on Oct 28, 2023
5. in depends/config.guess:144 in 92f7e7f363

   140@@ -141,7 +141,7 @@ set_cc_for_build() {
   141 # This is needed to find uname on a Pyramid OSx when run in the BSD universe.
   142 # (ghazi@noc.rutgers.edu 1994-08-24)
   143 if test -f /.attbin/uname ; then
   144-	PATH=$PATH:/.attbin ; export PATH
   

   ---

   ---

   

   TheCharlatan commented at 11:48 am on October 30, 2023:

   Is this actually needed? Seems like a completely arcane check and I’m not sure how this would be tokenized wrong.

   ---

   

   fanquake commented at 11:50 am on October 30, 2023:

   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.

   ---

   

   maaku commented at 9:34 am on October 31, 2023:

   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?

   ---

   

   TheCharlatan commented at 10:00 am on October 31, 2023:

   Is this actually fixing anything though?

   ---

   

   laanwj commented at 1:25 pm on November 3, 2023:

   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 😄
6. fanquake renamed this:
   [depends] Allow PATH with spaces in directory names.
   depends: Allow PATH with spaces in directory names.
   on Oct 30, 2023
7. DrahtBot added the label Build system on Oct 30, 2023
8. 
   laanwj commented at 1:48 pm on October 31, 2023: member

   Concept ACK
9. 
   maflcko commented at 1:03 pm on November 3, 2023: member

   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.
10. 
    kristapsk commented at 3:15 pm on November 3, 2023: contributor

    Concept ACK
11. fanquake referenced this in commit 1fbeeed23a on Nov 14, 2023
12. 
    maflcko commented at 11:11 am on November 28, 2023: member

    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:

     0diff --git a/ci/test/02_run_container.sh b/ci/test/02_run_container.sh
     1index edf8f2c30..f82bbcd23 100755
     2--- a/ci/test/02_run_container.sh
     3+++ b/ci/test/02_run_container.sh
     4@@ -70,7 +70,7 @@ if [ "$CI_OS_NAME" == "macos" ]; then
     5 fi
     6 
     7 CI_EXEC () {
     8-  $CI_EXEC_CMD_PREFIX bash -c "export PATH=${BINS_SCRATCH_DIR}:${BASE_ROOT_DIR}/ci/retry:\$PATH && cd \"${BASE_ROOT_DIR}\" && $*"
     9+  $CI_EXEC_CMD_PREFIX bash -c "export PATH=\"/path_with space:${BINS_SCRATCH_DIR}:${BASE_ROOT_DIR}/ci/retry:\$PATH\" && cd \"${BASE_ROOT_DIR}\" && $*"
    10 }
    11 export -f CI_EXEC
    12 
    

    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.

13. maflcko added the label Up for grabs on Dec 7, 2023
14. 
    maflcko commented at 2:53 pm on December 7, 2023: member

    Marked as up-for-grabs. Should be trivial to fixup and bring over the finish line.
15. 
    alfonsoromanz commented at 6:48 pm on January 11, 2024: contributor

    Hi @maflcko! I opened this PR for this issue: https://github.com/bitcoin/bitcoin/pull/29237
16. fanquake removed the label Up for grabs on Jan 11, 2024
17. fanquake closed this on Jan 11, 2024

    ---

18. fanquake referenced this in commit 17e33fb578 on Jan 15, 2024
19. PastaPastaPasta referenced this in commit cb5877bb46 on Oct 24, 2024
20. PastaPastaPasta referenced this in commit 37389c7d38 on Oct 25, 2024
21. bitcoin locked this on Jan 10, 2025

Contributors
maaku DrahtBot TheCharlatan fanquake laanwj maflcko kristapsk alfonsoromanz

Labels
Build system