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

Move Mill process working directory to sandbox #3367

Merged
merged 11 commits into from
Aug 16, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Aug 13, 2024

This makes os.pwd an empty directory: out/mill-worker-*/sandbox/ for Mill's server mode, out/mill-no-server/sandbox/ otherwise

In general, code outside of tasks should use millSourcePath, while code inside of tasks should use T.dest or the PathRefs given to it by upstream tasks. Accessing project files via os.pwd can result in tasks not invalidating when files changed.

Like #3347, this does not prevent people intentionally walking the filesystem to work mischieve, but it does help mitigate "accidental" usage of os.pwd to access project files. The original os.pwd is still made available as mill.api.WorkspaceRoot.workspaceRoot outside of tasks, or T.workspace inside of tasks, for the occasional scenarios that need it. But at least this adds some partial guardrails against accidentally using os.pwd in a way that subverts Mill's expectations, and the APIs that expose the workspace root can be appropriately documented to warn people against doing the wrong thing

@lihaoyi lihaoyi marked this pull request as draft August 14, 2024 18:21
@lihaoyi lihaoyi marked this pull request as ready for review August 16, 2024 04:07
@lihaoyi lihaoyi requested review from lefou and lolgab and removed request for lefou August 16, 2024 04:07
@lihaoyi lihaoyi mentioned this pull request Aug 16, 2024
@lefou
Copy link
Member

lefou commented Aug 16, 2024

I understand the motivation, yet I find it a bit uncommon for a CLI tool to not have the current working directory the same as the directory, from where I started the process. I have to make myself comfortable with this change, I guess.

So, is this only changing the pwd of the Mill server process? What's the pwd of the client? And what's the pwd, when Mill runs in --no-server mode?

@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 16, 2024

The client pwd doesn't change, but the user can't write any code that runs in the Mill client, so the pwd of the client doesn't really matter.

For -i/--no-server, we still always spawn a sub-process for the Mill build logic (as of #3363), and thus are able to control it's PWD. The sandbox for that lives in out/mill-no-server/, since it doesn't have a out/mill-worker-*/ folder to use.

It's definitely a bit unusual, but even today people shouldn't really be reading or writing stuff out of os.pwd, since it bypasses any logic around cache invalidation, watch-and-re-run, and breaks the isolation guarantees that come with T.dest. There is a workaround of copying stuff into os.pwd when the build starts, and it can probably be made kinda-sorta robust via interp.watch or something, if people really need to grandfather in old logic e.g. in the example/thirdparty/acyclic. But using os.pwd should not be the first thing people reach for, and should be made suitably inconvenient to use

@lihaoyi lihaoyi merged commit b3b2999 into com-lihaoyi:main Aug 16, 2024
30 checks passed
@lefou
Copy link
Member

lefou commented Aug 16, 2024

For -i/--no-server, we still always spawn a sub-process for the Mill build logic (as of #3363), and thus are able to control it's PWD. The sandbox for that lives in out/mill-no-server/, since it doesn't have a out/mill-worker-*/ folder to use.

We might need to add something unique to that path, e.g. the process ID, since it's possible to start many mill --no-server processes in parallel.

lihaoyi added a commit that referenced this pull request Aug 22, 2024
`os.pwd` is no longer the repo root as of
#3367, so we need to explicitly
ask for `mill.api.WorkspaceRoot.workspaceRoot`

This allows a `installLocalCache`d version of Mill to successfully be
used to load a project into IntelliJ via BSP, whereas in `main` that
fails due to the incorrect workspace directory
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