Use OS-Lib's pwdDynamicFunction to allow sandboxing of os.pwd, write sandboxing doc page, introduce RunModule#runner
#3479
Conversation
lihaoyi
changed the title
pwdDynamicFunction to allow sandboxing of os.pwdpwdDynamicFunction to allow sandboxing of os.pwd, write sandboxing doc page
lihaoyi
changed the title
pwdDynamicFunction to allow sandboxing of os.pwd, write sandboxing doc pagepwdDynamicFunction to allow sandboxing of os.pwd, write sandboxing doc page, introduce RunModule#runner
| @@ -39,8 +39,6 @@ jobs: | |||
|
|
|||
| steps: | |||
| - uses: actions/checkout@v4 | |||
| with: | |||
| fetch-depth: 0 | |||
There was a problem hiding this comment.
We need this, to derive the version from git reliably.
There was a problem hiding this comment.
We dont need it jn every job, we just need it i the jobs that do publishing, which arent affected by this change
|
|
||
| // Remove any empty `.dest/` folders to tidy up the filesystem. Otherwise | ||
| // tasks that call `os.proc` or `T.dest` without actually doing anything with | ||
| // the folder will leave empty folders around cluttering the disk | ||
| this.synchronized { | ||
| for { | ||
| p <- usedDest | ||
| if os.list.stream(p).headOption.isEmpty | ||
| // workers can be used after they return their value and | ||
| // may need to continue working with their `T.dest`, | ||
| if !isWorker | ||
| } os.remove(p) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this could break tasks, that export their dest dir on purpose. E.g. I use this sometimes to refer to working directories used by other task. Also the GenIdea uses empty dest dirs as unique output dirs when configuring the Java/Scala compiler.
mill/scalalib/src/mill/scalalib/GenIdeaModule.scala
Lines 34 to 36 in 6e35f6e
There was a problem hiding this comment.
Yeah i noticed that. The use cases i hit was in 11-module-run-task, and in how we used to save a folder for running Mill's own integrstion tests. Both were hacks and i found solutions. We always tell people not to write to upstream dest folders after all.
Could there be another way of marking those tasks that need the folder? e.g. putting a marker file in the folder like git requires (https://www.theserverside.com/), or defining another variant of Task that is just a scratchspace that preserves the folder for usage
There was a problem hiding this comment.
another alternstive is to create a folder inside .dest and export that instead, just to tell Mill that you have something in dest tou want to keep. But we really should have a more principled stance on mutating upstream dest folders, just like T.persistent adds some structure around keeping them around
There was a problem hiding this comment.
decided to revert this for now, can discuss it in a follow up
Also did some misc cleanup:
synchronizedEvalis now unnecessary since #3243 added a coarse grained lock around BSP, prettied up the#threadIdprefixes, remove empty.dest/folders for tidynessRunModule#runnercontinues the pattern estabilished withCoursierModule#defaultResolver: a convenient way to bundle up the various tasks related to running a modules code in a way that lets downstream callers pass things torunwhile being able to override the default configs (e.g.forkEnv,forkArgs, etc.) and without the hassle of wrapping things inT.tasks. If this pattern works out, it would serve as a great alternative to the "pass tasks as arguments to command" pattern we've used in the past. This makes the11-module-run-taskexample much nicerAlso more CI optimizations