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

Allow .mill-jvm-opts to interpolate environment variables, add .mill-opts #3841

Merged
merged 11 commits into from
Oct 25, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Oct 25, 2024

This PR extends .mill-jvm-opts to support interpolating JVM variables. This provides a bit of flexibility in allowing $PWD and $USER to be used when passing -Dproperty= to Mill. This could in theory be fleshed out even further to run arbitrary shell snippets, but for now I this should provide enough flexibility to meet the immediate needs.

Also added a .mill-opts, which is similar to .mill-jvm-opts but used for Mill arguments rather than JVM arguments. This provides a way to persistent required Mill configuration in a format that is similar to what we already have today (CLI args). This would be a common place to put things like --jobs=0.5C or --allow-positional or --no-build-lock.

Updated the integration tests and documentation

related discussions

@lihaoyi lihaoyi requested a review from lefou October 25, 2024 00:13
Comment on lines -658 to -684
|mill_jvm_opts=""
|init_mill_jvm_opts () {
| if [ -z $$MILL_JVM_OPTS_PATH ] ; then
| mill_jvm_opts_file=".mill-jvm-opts"
| else
| mill_jvm_opts_file=$$MILL_JVM_OPTS_PATH
| fi
|
| if [ -f "$$mill_jvm_opts_file" ] ; then
| # We need to append a newline at the end to fix
| # https://github.com/com-lihaoyi/mill/issues/2140
| newline="
|"
| mill_jvm_opts="$$(
| echo "$$newline" | cat "$$mill_jvm_opts_file" - | (
| while IFS= read line
| do
| mill_jvm_opts="$${mill_jvm_opts} $$(echo $$line | grep -v "^[[:space:]]*[#]")"
| done
| # we are in a sub-shell, so need to return it explicitly
| echo "$${mill_jvm_opts}"
| )
| )"
| mill_jvm_opts="$${mill_jvm_opts} -Dmill.jvm_opts_applied=true"
| fi
|}
|
Copy link
Member Author

Choose a reason for hiding this comment

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

@lefou do you remember what this stuff is? AFAIK, we currently do all MILL_JVM_OPTS processing in the mill client Java code, so not sure why we have to do some bash logic here

Copy link
Member Author

Choose a reason for hiding this comment

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

Also the relevant integration tests seem to pass with this stuff deleted

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it might be remnant from back when mill -i ran everything within a single process, so we needed to make sure it was initialized correctly before that process started. Should be unnecessary now

Copy link
Member

Choose a reason for hiding this comment

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

Exactly. This was prior to processing this in Java. This is a left over and should be no longer needed since we no longer run any task evaluation in the client JVM.

@lihaoyi lihaoyi merged commit 7e7a54f into com-lihaoyi:main Oct 25, 2024
24 checks passed
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.

I think we should also (and only) support the delimited version ${VAR}.

Also, since we now only have one place where this is implemented, we should also support files in the .config dir as we do for .mill-version vs. .config/mill-version.

Comment on lines -658 to -684
|mill_jvm_opts=""
|init_mill_jvm_opts () {
| if [ -z $$MILL_JVM_OPTS_PATH ] ; then
| mill_jvm_opts_file=".mill-jvm-opts"
| else
| mill_jvm_opts_file=$$MILL_JVM_OPTS_PATH
| fi
|
| if [ -f "$$mill_jvm_opts_file" ] ; then
| # We need to append a newline at the end to fix
| # https://github.com/com-lihaoyi/mill/issues/2140
| newline="
|"
| mill_jvm_opts="$$(
| echo "$$newline" | cat "$$mill_jvm_opts_file" - | (
| while IFS= read line
| do
| mill_jvm_opts="$${mill_jvm_opts} $$(echo $$line | grep -v "^[[:space:]]*[#]")"
| done
| # we are in a sub-shell, so need to return it explicitly
| echo "$${mill_jvm_opts}"
| )
| )"
| mill_jvm_opts="$${mill_jvm_opts} -Dmill.jvm_opts_applied=true"
| fi
|}
|
Copy link
Member

Choose a reason for hiding this comment

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

Exactly. This was prior to processing this in Java. This is a left over and should be no longer needed since we no longer run any task evaluation in the client JVM.

@lefou
Copy link
Member

lefou commented Oct 25, 2024

Also added a .mill-opts, which is similar to .mill-jvm-opts but used for Mill arguments rather than JVM arguments. This provides a way to persistent required Mill configuration in a format that is similar to what we already have today (CLI args). This would be a common place to put things like --jobs=0.5C or --allow-positional or --no-build-lock.

How does this behave when we find an opion like --jobs in that file and specified on the CLI? We should not fail then but let the CLI override the default value from the file.

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