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

Set correct mill home path. #1334

Merged
merged 3 commits into from
May 26, 2021
Merged

Set correct mill home path. #1334

merged 3 commits into from
May 26, 2021

Conversation

heksesang
Copy link
Contributor

No description provided.

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.

Thank you for pointing out the issue (on gitter) and providing a fix.

It's a bit hard to spot the real change between all these formatting changes. I can see that you just applied the project default formatting, so this is ok. Maybe next time, you can keep functional changes in a separate commit. Anyways, I think we have a special situation here.

Ammonite and mill uses the same commandline parser (mainargs). Mill re-uses the ammonite core configuration part, which is IMHO far from optimal but at least helps to avoid code duplication.

The issue which you are trying to fix here is that the ammonite home directory is not properly set to $HOME/.mill/ammonite (since the last command-line parser refactoring in mill). This means, we use the $HOME/.ammonite directory, which is the default for ammonite.

Unfortunately, you introduce a new issue with your fix. When a user decides to override ammonite home with --home command line option, this switch will be reset to mills default right after parsing.

Without further studying the mainargs API, I think the only option we have is to check whether the config value contains the ammonite default value (ammonite.main.Defaults.ammoniteHome) and only then reset it to mills default (mill.api.Ctx.defaultHome). But even then, we miss the case, where the user wishes to set it explicitly to $HOME/.ammonite.

So, unless there is some better way to apply default values in mainargs, there is no proper solution. At least not if we try to reuse the ammonite config class for parsing.

@heksesang
Copy link
Contributor Author

What is the reason that we use .mill/ammonite and not .ammonite as the default?

@lefou
Copy link
Member

lefou commented May 25, 2021

I don't know, but it's IMHO good design to not pollute the state / cache area of another application. Although mill scripts are technically ammonite scripts, they are executed in a different context. Using a different cache area reduces the chance of a conflict and gives the user more control.

@heksesang
Copy link
Contributor Author

Well, the simple solution would probably be to just add a case class MillConfig which contains the home (with default to our mill path) and an ammonite config, construct this with mainargs, then we overwrite the ammoniteCore with the home from our config?

@lefou
Copy link
Member

lefou commented May 25, 2021

Yeah, that might work, but I hope it fails for consistency reasons, as ammonite Config.Core config already defines a home. I don't know how mainargs handles multiple definitions of the same options. It should detect conflicts though. We proably need to mirror the ammonite core config (that part we still want to be customizable, e.g. the rather new --thin option is something we probably don't want to see exposed).

@heksesang
Copy link
Contributor Author

Tested it and it seemed to work fine adding the home arg to the MillConfig.

@lefou
Copy link
Member

lefou commented May 26, 2021

Just to clarify. After your PR:

  • the default home path of mill is $HOME/.mill/ammonite
  • mill accepts a --home command line option to change that path
    ** mill --home /home/user/.ammonite will use /home/user/.ammonite as home path
    ** mill --home my-local-home will use ./my-local-home (relative to the working directory) as home path

Is that all correct?

@heksesang
Copy link
Contributor Author

I have used the creation of the runtime jar as the marker, by having a Docker image where ~/.mill/ammonite contains rt-16.0.1.jar and these are the results:

  • mill --repl finds the existing runtime.
  • mill --repl --home ~/.ammonite creates the runtime at ~/.ammonite.
  • mill --repl --home my-local-home creates the runtime at ./my-local-home.

This should be consistent with your assertions. :)

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.

Very nice. Thank you for this pull request!

@lefou lefou merged commit a7f7276 into com-lihaoyi:main May 26, 2021
@lefou lefou added this to the after 0.9.7 milestone May 26, 2021
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