Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export base directory before loading config file #347

Conversation

LCvanDinteren
Copy link
Contributor

By exporting the base directory before loading config, it is possible to use the base dir within the config file.

@XECDesign
Copy link
Member

I suspect shellcheck wouldn't be happy with that and want the assignment and export on separate lines.

@LCvanDinteren
Copy link
Contributor Author

I suspect shellcheck wouldn't be happy with that and want the assignment and export on separate lines.

This same structure is used for many exports below, which is why I chose to keep in line with those lines.
Do you suggest something like this? :

if [ -z "${BASE_DIR}" ]; then
  BASE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
fi
export BASE_DIR

@XECDesign
Copy link
Member

I didn't know about shellcheck when I wrote the first version, so I wouldn't be surprised. Just trying to stick to better practices now.

In most case, an export and assignment on the same line is fine because there's no command substitution. In this case, because the assignment can fail, you'd want to catch that (which doesn't happen if you export on the same line).

I'm not sure the [ -z "${BASE_DIR}" ] would make sense. What would be the use case for setting a BASE_DIR that's outside of build.sh? Has that use case been tested to work with that change or would it also be broken in other ways?

I think if you move the existing BASE_DIR= and export BASE_DIR lines up so that's they're after the root user check, it should be fine.

@LCvanDinteren
Copy link
Contributor Author

Alright, always a fan of better practices:)

I can't think of any use for defining BASE_DIR outside of build.sh, but figured that as I'm changing it anyway, might as well align it with the rest of the code.

Updated as per your suggestion.

@XECDesign
Copy link
Member

Just one last thing. There's a stray export BASE_DIR here

pi-gen/build.sh

Line 183 in 97076ab

export BASE_DIR

@LCvanDinteren
Copy link
Contributor Author

Nice catch, my bad!

@XECDesign XECDesign merged commit 7f143a1 into RPi-Distro:master Nov 18, 2019
@XECDesign
Copy link
Member

Many thanks

@LCvanDinteren
Copy link
Contributor Author

Many thanks to you as well!

PeterJohnson pushed a commit to PeterJohnson/WPILibPi that referenced this pull request Dec 7, 2019
edurenye pushed a commit to edurenye/pigall-gen that referenced this pull request May 20, 2020
alexgg pushed a commit to balena-os/pi-gen that referenced this pull request Jul 12, 2021
UmeshMohan-Dozee pushed a commit to DozeeRnD/pi-gen that referenced this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants