-
Notifications
You must be signed in to change notification settings - Fork 50
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
Environment variables for setup and test tasks #215
Conversation
I'm not really sure if this is the right way to do it, so please be zealous in reviewing it 😄 |
any update here? |
Yeah... that's my bad. Let me get to it this week. |
thx |
options: cp.ExecFileOptions = { shell: true }, | ||
) { | ||
if (extraEnv) { | ||
options.env = { ...(options.env ?? process.env), ...extraEnv }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I don't understand the node API here, if we pass an environment object, does it not also inherit the parent environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure. The documentation says, about the env
option:
env
: Environment key-value pairs. Default:process.env
.
So because the default is process.env
and not {}
, I'd guess that options.env
replaces the parent environment. (That also makes sense because otherwise, you couldn't delete environment variables.)
Just a few comments, and then it should be good to go. It would also be useful if you could add an entry in the CHANGELOG.md file. Otherwise, I can get to it after merge. |
This setting allows setting extra environment variables during the setup stage.
It's already possible to add environment variables in the 'options' argument — but child_process.execFile() does not treat options.env as extra environment variables to add. It replaces the environment with options.env entirely. So, it's convenient to have an extraEnv argument with variables to add. This will be necessary for the testEnvironment setting added in the next commit.
This setting allows setting extra environment variables while running meson test, as well as individual tests.
Consistently replace 'Meson setup' -> 'setup' as pointed out in code review. Improve phrasing for mesonbuild.buildFolder.
d02cfce
to
f44631d
Compare
This adds two options,
configureEnvironment
andtestEnvironment
, allowing users to set particular environment variables duringmeson setup
andmeson test
.Closes: #9