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 users to use a custom output dir via an env var #3530

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

alexarchambault
Copy link
Contributor

@alexarchambault alexarchambault commented Sep 12, 2024

This allows users to change the Mill output directory (by default, out under the project root) to a directory of their choice, via the MILL_OUTPUT_DIR environment variable.

Fixes #3144

@alexarchambault

This comment was marked as duplicate.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 12, 2024

How about we use an env var rather than a flag? That way we dont get to parse it and can keep the server files in the same place (relatively) as they are today. I'd rather not split them into a separate folder if it can be avoided

@lihaoyi
Copy link
Member

lihaoyi commented Sep 12, 2024

env var isnt perfect either from a UX perspective, but given this is a bit of a niche case I think its a better solution than reorganizing the file layout for this feature due to implementation concerns.

lets go with an envvar for now and at some point we may have a graal native or scalanative client and can have it be smart enough to use flags

@lefou
Copy link
Member

lefou commented Sep 13, 2024

As for the cli args, if at all, I'd suggest to just go with --output. It's a bit too early to settle on a short option. Short options are scarce resources. Otherwise, I think using a env var is ok for the start, too.

Here are some more motivations for alternative out dirs:

  • read-only project dirs
  • project file systems unsuitable to keep run-state of a process
  • out dirs in RAM filesystems

As a consequence, I think we should be able to split the two uses of out, Mill target cache vs Mill worker state. I'd really like to be able to keep all Mill worker state (of arbitrary projects) in one single location. e.g. something in $XDG_RUNTIME_DIR. Most code should be already prepared for this, since we made it independent of os.pwd. This would also avoid the issue, that mill currently creates a ./out dir in every dir it is started from.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 13, 2024

Let's go with the env var for now and keep the out/ folder intact, at least as part of this ticket. I think that "all generated files in the out/ folder" is a very nice property to have, even though I had different ideas initially (e.g. our integration test fixtures used to separate them build-generated-files and misc-housekeeping-files), and even though today it's not strictly true (we dump some stuff in ~/.cache as well).

I do agree that generating an ./out folder in every dir that Mill starts in isn't great, and we may still want to break it up later, but we don't need to start that right now. e.g. I could imagine that with graal/scala-native allowing a thicker client, and some kind of overhaul of the evaluator command system, the problem may solve itself in a nicer way. But that will take more time to research and experiment

@alexarchambault alexarchambault force-pushed the custom-out branch 4 times, most recently from 365dede to b5768b4 Compare September 23, 2024 09:52
@alexarchambault alexarchambault marked this pull request as ready for review September 23, 2024 09:54
@alexarchambault
Copy link
Contributor Author

I went for the MILL_OUTPUT environment variable for now

@lefou
Copy link
Member

lefou commented Sep 23, 2024

I went for the MILL_OUTPUT environment variable for now

For looking at my env output, most names of variables holding a path also end in PATH.

@alexarchambault
Copy link
Contributor Author

I'd go for something like MILL_OUTPUT, MILL_OUTPUT_DIRECTORY, or MILL_OUTPUT_FOLDER. Env var ending in PATH evoke Unix's PATH, LD_LIBRARY_PATH, or Java's CLASSPATH, which all accept a : (or ;) separated list of directories, which we don't here.

@lefou
Copy link
Member

lefou commented Sep 23, 2024

Then my favorite is MILL_OUTPUT_DIR, DIRECTORY is too long and FOLDER is a Windows term.

@alexarchambault alexarchambault force-pushed the custom-out branch 2 times, most recently from b3ad81b to 2dcda66 Compare September 23, 2024 13:12
@alexarchambault alexarchambault changed the title Allow users to pass a custom output dir on the command-line Allow users to use a custom output dir via an env var Sep 23, 2024
@lihaoyi
Copy link
Member

lihaoyi commented Sep 24, 2024

I think this looks great. As a last thing before landing, let's convert the integration test into an example test in example/depth/ (not sure what subfolder, maybe a new one?) and include it as part of a dedicated section on the Out_Dir.adoc page of our doc-site with a write-up.

You may need to hack ExampleTester to allow stuff like MILL_OUTPUT_DIR=foo ./mill blah to work properly, but that's probably fine, and it would ensure that the documentation and tests stay in sync and that the feature is discoverable to our users

@alexarchambault
Copy link
Contributor Author

As a last thing before landing, let's convert the integration test into an example test in example/depth/ (not sure what subfolder, maybe a new one?) and include it as part of a dedicated section on the Out_Dir.adoc page of our doc-site with a write-up.

Just added an example under example/depth/out-dir/1-custom-out, and added it to Out_Dir.adoc. I kept the integration tests, as more things can be tested from them (out dir outside of the workspace for example), and their checks are more precise than the grepping done in the examples.

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Sep 24, 2024

You may need to hack ExampleTester to allow stuff like MILL_OUTPUT_DIR=foo ./mill blah to work properly, but that's probably fine, and it would ensure that the documentation and tests stay in sync and that the feature is discoverable to our users

That ended up not being needed, as these commands are run through bash -c (even on Windows apparently, using Git Bash), and specifying env vars this way is valid bash.

Copy link
Member

@lihaoyi lihaoyi left a comment

Choose a reason for hiding this comment

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

Looks great, @alexarchambault just need to update the PR description to reflect the current state of the PR and we can merge it

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Sep 26, 2024

(Last push is just a rebase on the latest main, to address conflicts)

@lihaoyi lihaoyi merged commit 4c3d2cb into com-lihaoyi:main Sep 26, 2024
24 checks passed
@lefou lefou added this to the 0.12.0-RC3 milestone Sep 26, 2024
@alexarchambault alexarchambault deleted the custom-out branch September 26, 2024 10:27
lihaoyi added a commit that referenced this pull request Oct 2, 2024
#3637)

Follows suit from #3530. This
hasn't been released in a stable version yet so we are free to change it
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.

Support alternative/configurable out directory
3 participants