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

Make .mill-opts/.mill-jvm-opts env var interpolation require braces, add tests for overriding opts #3844

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Oct 25, 2024

No description provided.

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 25, 2024

CC @lefou

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PWD is the client working dir, right?

We seem to interpolate missing env vars to the empty String. Shouldn't we at least print some warning to STDERR?

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 25, 2024

Yeah PWD is client working dir

Not sure about printing an error; having optional environment variables seems like it would be a common use case, in which case it's not an error for the env var to be missing

@lefou
Copy link
Member

lefou commented Oct 25, 2024

Not sure about printing an error; having optional environment variables seems like it would be a common use case, in which case it's not an error for the env var to be missing

I find some kind of defensive programming can avoid lots of hard to spot issues. We could just log it when we run in --debug mode. But I think we don't fully parse the cli in the client. (If we do, we should also print which additional files we read in --debug mode. Can be another PR though.)

@lefou
Copy link
Member

lefou commented Oct 25, 2024

If we don't log it, we should document what happens with undefined env vars.

@lefou
Copy link
Member

lefou commented Oct 25, 2024

We don't interpolate .mill-vars .mill-opts currently. But I think we should. For consistency, but also because it's probably the natural way to pass env vars to system properties.

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 25, 2024

@lefou what's .mill-vars?

@lefou
Copy link
Member

lefou commented Oct 25, 2024

@lefou what's .mill-vars?

A typo. I meant .mill-opts.

@lihaoyi lihaoyi merged commit 2448e81 into com-lihaoyi:main Oct 25, 2024
24 checks passed
@lefou lefou added this to the 0.12.1 milestone Oct 25, 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