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

Look for .mill-version in .config directory #945

Merged
merged 3 commits into from
Jul 21, 2022

Conversation

nrktkt
Copy link
Contributor

@nrktkt nrktkt commented Aug 6, 2020

More of a conversation starter than something to merge right away.

There's an initiative at nodejs/tooling#79 to decrease the number of configuration files in a repo root. I generally like the idea. If that gets finalized I propose we merge this.
It still checks for the file in both the repo root and the .config directory, so it seems to me we'd gain an option and there isn't much to lose.

@Ammonite-Bot
Copy link
Collaborator

Ammonite-Bot commented Aug 6, 2020 via email

@joan38
Copy link
Collaborator

joan38 commented Aug 18, 2020

Personally I don't understand the point of using .mill-version instead of mill.
The mill script let people:

  • Use mill without installing it
  • Act as a .mill-version file even if you run mill without the script
  • You can add things like:
    export COURSIER_REPOSITORIES="ivy2Local|https://artifacts.my-company.com"
    export JAVA_OPTS="-Xms2g -Xmx2g -Xss4m -XX:ReservedCodeCacheSize=512m -XX:MaxMetaspaceSize=512m"
    

@nrktkt
Copy link
Contributor Author

nrktkt commented Aug 18, 2020

@joan38

  • the mill script provides a default version, which can be overridden by any of several settings. .mill-version sets the version explicitly and will not be overridden if present.
  • tooling (like metals) will respect .mill-version, but will not notice the mill wrapper script.
  • developers coming into a project who already have mill installed are likely to use mill instead of ./mill.

regardless, this issue isn't about the existence of .mill-version, it's about the behavior.

@nrktkt
Copy link
Contributor Author

nrktkt commented Aug 18, 2020

@Ammonite-Bot

Personally I’d say we should wait for the dust to settle before we hope on
board.

agreed, we can leave this issue open as things develop.

see where the rest of the community (not just nodejs) settles

I lean towards being an early adopter rather than in the early majority, given that it's an optional setting. but that will probably still be a while.

@joan38
Copy link
Collaborator

joan38 commented Aug 18, 2020

the mill script provides a default version, which can be overridden by any of several settings. .mill-version sets the version explicitly and will not be overridden if present.

Not sure if that's a pro or a con.

tooling (like metals) will respect .mill-version, but will not notice the mill wrapper script

Tooling should be using BSP. I use Intellij through BSP and it runs the command described in .bsp/mill.json.
It also forwards any env vars including COURSIER_REPOSITORIES and JAVA_OPTS.

developers coming into a project who already have mill installed are likely to use mill instead of ./mill.

This point is not a pro since using mill will still pick the version in ./mill.

Overall I usually try to have the min of files/mess especially at the root of the project. With SBT we used to have at least build.sbt, .sbtopts, project/build.properties, project/plugins.sbt. Now we have only build.sc and ./mill.

BTW, is @Ammonite-Bot a clone of @lihaoyi ? Is that how you manage to bend the time and do so much in the Scala community, by demultiplicating yourself @lihaoyi? 😄

@lefou
Copy link
Member

lefou commented Aug 20, 2020

@joan38 .mill-version is a config file. ./mill is a script. IMHO it's better to consult some config file to get a value than executing some arbitrary script and hoping for the right outcome. Also my system wide installed mill executable is https://github.com/lefou/millw and it will not look into any mill script to infer the right version.

If we need to have more than one config value per project, we can discuss to change from .mill-version to .millrc or something like that, like any other linux/unix tool does.

@lefou
Copy link
Member

lefou commented Sep 2, 2020

See also #954

@lefou lefou marked this pull request as draft December 8, 2020 20:35
@lefou
Copy link
Member

lefou commented Jul 20, 2022

Some time has passed, yet https://dot-config.github.io/ is only supported by a very small set of tools. If we decide to be an early adopter, here are my concerns / change requests:

Once we use a .config/ directory, which is already hidden, we should not make the config file itself also hidden. It should be .config/mill-version and not .config/.mill-version. This is also how it is handled in XDG specified $HOME/.config.

Additionally, we should give .mill-version more priority than .config/mill-version. So, early adopter can start using .config/, but once there is a .mill-version file, it will take precedence, to not confuse users and tools.

@nrktkt
Copy link
Contributor Author

nrktkt commented Jul 20, 2022

I think those changes are reasonable

@nrktkt nrktkt marked this pull request as ready for review July 20, 2022 19:25
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.

Looks good to me. I think, we don't risk much by supporting this.
Would be nice, to have this also in millw for consistency.

@lefou lefou merged commit 1e025cd into com-lihaoyi:main Jul 21, 2022
@lefou lefou added this to the after 0.10.5 milestone Jul 21, 2022
nrktkt added a commit to nrktkt/millw that referenced this pull request Jul 21, 2022
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.

4 participants