- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 420
Read coursier env vars via Task.Input #5350
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most tricky case with long running daemon is the case where the env variable is defined initially, but later gets removed from the env, in which case we need to fall back to the other defaults. Do we have this in a test case already?
        
          
                libs/scalalib/src/mill/scalalib/dependency/versions/VersionsFinder.scala
              
                Outdated
          
            Show resolved
            Hide resolved
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Found two other nits.
Could you also clean installation-ide.adoc which references the issues you just fixed? 
mill/website/docs/modules/ROOT/pages/cli/installation-ide.adoc
Lines 277 to 278 in 2ee59e4
| NOTE: Currently, you may need to `./mill shutdown` before calling Mill with the environment variable to make sure it gets properly propagated to Mill's background daemon. | |
| There is an https://github.com/com-lihaoyi/mill/issues/5134[open issue] to let Mill detect these changes automatically. | 
| /** | ||
| * Aggregates coursier parameters read from the environment | ||
| */ | ||
| def coursierConfig: Task[mill.util.CoursierConfig] = Task.Anon { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a candidate for caching, to me.
| def coursierConfig: Task[mill.util.CoursierConfig] = Task.Anon { | |
| def coursierConfig: Task[mill.util.CoursierConfig] = Task { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been trying that. Some abstract classes are involved (coursier.core.Repository, coursier.params.Mirror, coursier.credentials.Credentials in particular), which can be worked around, given this class is parsed from a bunch of strings, and the class instances are actually only simple easy-to-serialize classes.
But it seems this can also involve credentials (parsed from files on disk in particular). This raises security concerns: we don't adjust permissions of files where task results are written, so we could end up writing passwords in files with too loose permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it as-is then. Would be nice to have some support for secret caching (e.g. via encryption, or just in memory), but we currently don't.
        
          
                libs/scalalib/src/mill/scalalib/dependency/versions/VersionsFinder.scala
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                libs/scalalib/src/mill/scalalib/dependency/versions/VersionsFinder.scala
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                libs/scalalib/src/mill/scalalib/dependency/DependencyUpdatesImpl.scala
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | I think this looks fine, once the last set of review comments have been resolve we can merge this | 
Conflicts: core/util/src/mill/util/Jvm.scala mill-build/src/millbuild/Deps.scala
This reverts commit d03fd45.
…m-lihaoyi#5404) This reverts commit d03fd45. Fixes com-lihaoyi#5398
This is a new attempt at fixing #5134. Turns out setting the context class loader when evaluating user code addresses the issue while not running into #5398 again. --- Description from #5350 that still applies: This makes Mill read all environment variables and Java properties used by coursier via a `Task.Input`. In more detail, this adds a `CoursierConfigModule` external module in scalalib. It contains a single `Task.Input`, that reads all environment variables used by coursier via `Task.env` and all Java properties used by coursier via `sys.props`. Helper tasks compute default values for various parameters (repositories, cache location, etc.). `CoursierConfigModule.coursierConfig` returns most of these parameters in a case class defined in Mill, `CoursierConfig`. For convenience, down-the-line, we pass `CoursierConfig` instances around. Fixes #5134 (by managing coursier env vars / Java properties via `Task.Input`)
This makes Mill read all environment variables and Java properties used by coursier via a
Task.Input.In more detail, this adds a
CoursierConfigModuleexternal module in scalalib. It contains a singleTask.Input, that reads all environment variables used by coursier viaTask.envand all Java properties used by coursier viasys.props.Helper tasks compute default values for various parameters (repositories, cache location, etc.).
CoursierConfigModule.coursierConfigreturns most of these parameters in a case class defined in Mill,CoursierConfig. For convenience, down-the-line, we passCoursierConfiginstances around.Fixes #5134 (by managing coursier env vars / Java properties via
Task.Input)Fixes #5229 (via coursier/coursier#3426 from coursier
2.1.25-M14and the small rework of default repository handling)