ci: Cleanup ci/test/01_base_install.sh #27321

1. 
   maflcko commented at 12:41 pm on March 24, 2023: member

   Motivation

   Currently ci/test/01_base_install.sh has the following issues:

   • Two names to alias bash -c: CI_EXEC_ROOT, and CI_EXEC
   • The aliases are not needed, as explained in the comment in the file

   Thus, they should be removed, or at least de-duplicated. The whole file is already run in bash, so the only place where bash -c "..." is needed is in the apt install step to properly pass the packages string with spaces.

   Possible solution

   • Remove or de-duplicate CI_EXEC_ROOT, and CI_EXEC

   Useful Skills

   • Bash
   • Docker or Podman
   • The Bitcoin Core ./ci/ system (see Readme there). For testing I recommend to set up a fresh Linux VM.

   Guidance for new contributors

   Want to work on this issue?

   For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

2. maflcko added the label good first issue on Mar 24, 2023
3. maflcko added the label Tests on Mar 24, 2023
4. 
   Ayush170-Future commented at 6:48 pm on March 25, 2023: contributor

   Hello @MarcoFalke! I’m new to Bitcoin Core development as well as Bash scripting. But, based on the requirements and the script, I believe I can tackle this.

   I only have a few questions:

   1. I discovered other files in the test directory, such as ci/test/04_install, also contain the CI_EXEC_ROOT and CI_EXEC alias. So, are they also to be modified?

   2. After reading your comment in the script, I believe I can just remove the CI_EXEC_ROOT and CI_EXEC parts of different commands in the script and it will run just fine, as it is already run in bash. Except, for the apt install parts where I have to remove the alias and replace it with bash -c, as you said. So, am I thinking in the right direction?

   I can start working on this from Monday. So, if this is urgent or anyone else thinks they are more qualified than me to work on this, I would love to do a code review on their PR as well. I just want to get started and be involved.

5. 
   vstoyanov commented at 12:40 pm on March 26, 2023: none

   Hello,

   I think I just completed this one. My understanding was that it only covers 01_base_install.sh and not instances after test/04_install.sh as from there on it also redefines $PATH and current directory.

   Best, Vasil Stoyanov

   P.S. Apologies, @Ayush170-Future, I didn’t see your comment. Basically, I did the code change yesterday afternoon and today afternoon I only ran export CONTAINER_NAME=test1 && ./ci/test_run_all.sh. I really hope you hadn’t done any work already.

6. vstoyanov referenced this in commit bce5707dfd on Mar 26, 2023
7. vstoyanov referenced this in commit 1ba1b4fc92 on Mar 26, 2023
8. 
   Ayush170-Future commented at 3:02 pm on March 26, 2023: contributor

   Hey @vstoyanov! No problem at all. You seem to understand this far better than I do, so no worries. I will at the very least do a code review for your PR (as I mentioned in my comment). And will look for some other issues to contribute to.

   P.S. Thanks for clearing my test/04_install.sh doubt. I got a little confused there.

9. 
   vstoyanov commented at 3:43 pm on March 26, 2023: none

   @Ayush170-Future Thanks for understanding. Also thanks for the complement, but maybe this is not the case :) - I got to the point where $CI_BASE_PACKAGES and $CI_PACKAGES should pass lint. This means either convert them to bash arrays or use SuppressWarnings to switch lint off. The first seems much cleaner to do, the second is a smaller change and I am confused as to what to do with that.

   In about 45 minutes I will push the array conversion in a second commit I could squash later if we decide to go that way. Otherwise we could just Suppress Warnings for ${CI_RETRY_EXE} dnf -y --allowerasing install "${CI_BASE_PACKAGES[@]}" "${PACKAGES[@]}"

10. 
    Ferrydh commented at 3:46 pm on March 26, 2023: none

    这是来自QQ邮箱的假期自动回复邮件。   您好，我最近正在休假中，无法亲自回复您的邮件。我将在假期结束后，尽快给您回复。
11. vstoyanov referenced this in commit a45ee17792 on Mar 26, 2023
12. vstoyanov referenced this in commit f52d482408 on Mar 26, 2023
13. 
    vstoyanov commented at 5:38 pm on March 26, 2023: none

    False alarm. The bash -c hack also worked for dnf the same way it did for apt, so now it is both not a big change and has no SuppressWarnings BS. I could still push the arrayification of $PACKAGES and $CI_BASE_PACKAGES though? @Ferrydh Enjoy your vacation :)
14. 
    Ayush170-Future commented at 7:29 pm on March 26, 2023: contributor

    I guess SuppressWarnings was not a good practice. It’s great that you found a workaround.

    Instead of passing package strings as an array, I believe the bash -c technique should be sufficient here. @MarcoFalke, I think you wanted it that way as well.

15. vstoyanov referenced this in commit 15701793e1 on Mar 27, 2023
16. vstoyanov referenced this in commit 5d8b526c43 on Mar 27, 2023
17. vstoyanov referenced this in commit a66a86ba74 on Mar 27, 2023
18. vstoyanov referenced this in commit b5ef1419ec on Mar 27, 2023
19. fanquake closed this on Mar 30, 2023

    ---

20. sidhujag referenced this in commit 99235b18a0 on Apr 1, 2023
21. RandyMcMillan referenced this in commit 3139af950f on May 27, 2023
22. bitcoin locked this on Mar 29, 2024

Contributors
maflcko Ayush170-Future vstoyanov Ferrydh

Labels
Tests good first issue