Skip to content

dev: add mise config#6081

Closed
neongreen wants to merge 1 commit into
mainfrom
emily/jj-xxsmosnkoonr
Closed

dev: add mise config#6081
neongreen wants to merge 1 commit into
mainfrom
emily/jj-xxsmosnkoonr

Conversation

@neongreen
Copy link
Copy Markdown
Contributor

This replaces #5636.

After setting up Mise, we can use mise run to execute tasks. Mise also takes care of installing uv.

I didn't add rust/cargo to mise tools, because the mise repo itself doesn't do that. Maybe they have a good reason.

TODO:

  • Try everything on Windows

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@neongreen neongreen requested a review from ilyagr March 18, 2025 22:28
Comment thread mise.toml Outdated
Comment on lines +48 to +50
# TODO: this seems to run the same tests as the previous command,
# plus the insta tests. Do we need both?
"cargo insta test --workspace --test-runner nextest",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand nextest vs insta

Comment thread mise.toml Outdated
Comment thread Cargo.toml Outdated
@neongreen neongreen force-pushed the emily/jj-xxsmosnkoonr branch from 21e96ba to f7aa20f Compare March 18, 2025 22:40
@neongreen neongreen requested a review from arxanas March 18, 2025 23:00
@neongreen
Copy link
Copy Markdown
Contributor Author

fwiw i've been using mise for several months by now (thanks to @arxanas's rec? or someone's rec?) and i like it a lot

Copy link
Copy Markdown
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor stuff, still split on it if we actually want it,

Comment thread mise.toml Outdated
Comment thread mise.toml Outdated
Comment on lines +47 to +41
# TODO: this seems to run the same tests as the previous command,
# plus the insta tests. Do we need both?
"cargo +1.84 insta test --workspace --test-runner nextest",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we need both and this is the correct invocation to run all tests, so delete L46.

Comment thread mise.toml Outdated
# We want rustfmt from nightly
"rustup toolchain install nightly --profile minimal --component rustfmt",
# Tools installed via Cargo
"cargo +1.84 install --locked bacon",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: do we really want the filewatcher here, since I at least haven't had the need to use it yet?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use mise itself to watch files and run tasks, if that's appropriate here?

Comment thread mise.toml
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently is missing the documentation update in contributing.md

@arxanas arxanas mentioned this pull request Mar 29, 2025
4 tasks
Copy link
Copy Markdown
Contributor

@arxanas arxanas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I'm in favor, pending the outstanding comments.

  • We could potentially use mise to unify CI workflows as well, as I mentioned in a review comment.
  • We should of course adopt at most one task-runner system, so we'd have to oust mise if we wanted to use something else later.
  • I'm in favor of mise since, in addition to serving as a task runner, it also solves several cross-language devtool problems, unlike e.g. cargo xtask.
    • Using mise might concretely make the cross-language formatting problem much easier to solve (#3757).

Comment thread mise.toml Outdated
uv = "0.5.1"

[tasks."setup"]
description = "Set up the dev environment (install Rust, etc)"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: At this point, if using mise, should we just manage the Rust toolchain with it as well? That is, just do mise use --pin rust@1.84.

I'm not sure if there's a way to install two versions of the same tool in the same project for the cases where we want to use nightly, but it might work to just define the task to run mise exec rust@nightly -- cargo ... instead of cargo +nightly ...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I see you wrote

I didn't add rust/cargo to mise tools, because the mise repo itself doesn't do that. Maybe they have a good reason.

I didn't see that mise pins the version of Rust via rust-toolchain.toml, either, so maybe that's the difference?

I'm in favor of pinning the Rust version, as per my last attempt (reverted due to a GitHub Actions issue), but it's fine to not do that, too, if it would be complicated to support the nightly commands we want to run. (But, as per my other comment in this thread, it looks like managing Rust/Cargo via mise would also make it easier to manage cargo-insta and cargo-nextest.)

Copy link
Copy Markdown
Contributor

@ilyagr ilyagr Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering whether mise use could support using a different version of Rust for cargo fmt than for everything else.

Update: Oh, it seems you figured this out with #6081 (comment).

Comment thread mise.toml Outdated
# We want rustfmt from nightly
"rustup toolchain install nightly --profile minimal --component rustfmt",
# Tools installed via Cargo
"cargo +1.84 install --locked bacon",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use mise itself to watch files and run tasks, if that's appropriate here?

Comment thread mise.toml Outdated
Comment thread mise.toml Outdated
Comment thread mise.toml Outdated
description = "Lint the code"
run = "cargo +1.84 clippy --workspace --all-targets"

[tasks."lint:ci"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The usage of : is semantically-meaningful since it separates wildcard boundaries (see Task grouping).

If you run mise run lint:*, you might expect that to run "all" lints, but it'll actually exclude the most common linting use-case. I would try to organize the groups such that they're hierarchical in some sense to make it a little more intuitive.

suggestion (non-blocking): I've been organizing my tasks roughly by function rather than domain, like build:*, check:*, fix:*, etc.

  • Then I also rely on mise's parallelization features (after manually, painstakingly setting the dependencies between tasks).
  • In particular, it's useful to separate read-only tasks from read-write tasks, since you don't want writers to clobber each other when running in parallel.

Here's one possible organization, but I don't feel strongly, and I don't mean to suggest that the unimplemented commands below need to be implemented:

  • build:cli-reference
  • build:docs
  • build:debug
  • build:release
  • check:lint
  • check:format
  • check:test
  • check:zizmor
  • ci:lint
  • fix:format
  • fix:lint
  • serve:docs

Comment thread mise.toml
''',
]

[tasks."zizmor"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: zizmor is the specific tool name, while the rest of the task names are pretty abstract. I'd rename this to perhaps check:workflows (as per above), or rename other stuff to e.g. clippy.

Comment thread mise.toml Outdated
Comment on lines +14 to +20
"cargo +1.84 install --locked cargo-insta",
"cargo +1.84 install --locked cargo-nextest",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment (non-blocking): You might be able to

  • use/abuse sources/outputs
  • define the cargo install command(s) as its own task
  • make the test task depend on the installation command(s)

in order to get a one-step mise run test install+develop experience. (It's no Nix, but it might work? I haven't specifically tried the above.)

Copy link
Copy Markdown
Contributor

@arxanas arxanas Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interestingly, cargo insta is itself a mise tool: https://github.com/jdx/mise/blob/c67be43fcede947920c8eb17afb059037235a3e0/mise.toml#L12. From trying it out, it looks like cargo nextest is as well.

It could be good to add and pin those, so that we don't get contributors clobbering each others' insta snapshots when they occasionally make changes to the output format.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff seems to work for me in order to register all the tools directly with mise and support a one-command workflow:

diff --git a/mise.toml b/mise.toml
index 3d90390bc..07fd1bc73 100644
--- a/mise.toml
+++ b/mise.toml
@@ -1,27 +1,19 @@
 #:schema https://mise.jdx.dev/schema/mise.json
 
 [tools]
+"cargo:cargo-insta" = "1.42.2"
+"cargo:cargo-nextest" = "0.9.93"
+rust = "1.84"
 uv = "0.5.1"
 
-[tasks."setup"]
-description = "Set up the dev environment (install Rust, etc)"
-run = [
-  "rustup toolchain install 1.84",
-  # We want rustfmt from nightly
-  "rustup toolchain install nightly --profile minimal --component rustfmt",
-  # Tools installed via Cargo
-  "cargo +1.84 install --locked bacon",
-  "cargo +1.84 install --locked cargo-insta",
-  "cargo +1.84 install --locked cargo-nextest",
-]
-
 [tasks."format"]
 description = "Format the code"
-run = "cargo +nightly fmt"
+tools.rust = "nightly"
+run = "cargo fmt"
 
 [tasks."lint"]
 description = "Lint the code"
-run = "cargo +1.84 clippy --workspace --all-targets"
+run = "cargo clippy --workspace --all-targets"
 
 [tasks."lint:ci"]
 description = "Lint the code using the same clippy version as CI would"
@@ -42,16 +34,11 @@ run = "uvx zizmor ."
 
 [tasks."test"]
 description = "Run tests"
-run = [
-  "cargo +1.84 nextest run --workspace",
-  # TODO: this seems to run the same tests as the previous command,
-  # plus the insta tests. Do we need both?
-  "cargo +1.84 insta test --workspace --test-runner nextest",
-]
+run = "cargo insta test --workspace --test-runner nextest"
 
 [tasks."update-cli-help"]
 description = "Update generated CLI help (cli/tests/cli-reference@.md.snap)"
-run = "cargo +1.84 insta test --accept --workspace -- test_generate_md_cli_help"
+run = "cargo insta test --accept --workspace -- test_generate_md_cli_help"
 
 [tasks."docs:build"]
 description = "Build documentation into rendered-docs/ for offline use"

Note: It looks like uv is installed even though it's not necessary except for the uv tasks.

  • It might work to declare individual tools for the uv commands, which presumably would cause them to lazily provision the tools.
  • Perhaps the uv version could be deduplicated between the two commands via vars, rather than inlined.
  • (Hmm, if only there were a system which could represent a fine-grained, cross-language build graph...)

Copy link
Copy Markdown
Contributor

@ilyagr ilyagr Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I was wondering about similar questions and how far it can go with mise, but I haven't gotten around to looking at it in detail.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to set up rust via mise since even the author of mise doesn't do that. But we can try it out and see what happens

I'll try out your patch on mac and windows

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re/ vars -- they don't work in task tools, see jdx/mise#4885

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re/ rust versions -- different versions per tool seem to work just fine unless global tools.rust is set

Comment thread Cargo.toml
version = "0.27.0"
license = "Apache-2.0"
rust-version = "1.84" # NOTE: remember to update CI, contributing.md, changelog.md, and install-and-setup.md
rust-version = "1.84" # NOTE: remember to update mise.toml, CI, contributing.md, changelog.md, and install-and-setup.md
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rust-version = "1.84" # NOTE: remember to update mise.toml, CI, contributing.md, changelog.md, and install-and-setup.md
rust-version = "1.84" # NOTE: remember to update CI, mise.toml contributing.md, changelog.md, and install-and-setup.md

nit: I feel like "CI" should go first since it's most important, in some sense.

Comment thread mise.toml Outdated
[tasks."setup"]
description = "Set up the dev environment (install Rust, etc)"
run = [
"rustup toolchain install 1.84",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Non-blocking) This might be controversial, but I'd just use stable. Rust is getting pretty good at backward-compatibility, e.g. clippy is aware of which version of Rust is the MSRV and doesn't suggest inappropriate hints. OTOH, refactoring tools and clippy itself get better in newer versions, as does compilation speed, etc.

@ilyagr ilyagr mentioned this pull request Apr 2, 2025
4 tasks
@neongreen neongreen force-pushed the emily/jj-xxsmosnkoonr branch from f7aa20f to 77a7a6b Compare April 11, 2025 16:28
@neongreen neongreen requested a review from a team as a code owner April 11, 2025 16:28
@neongreen neongreen marked this pull request as draft April 11, 2025 19:05
@neongreen
Copy link
Copy Markdown
Contributor Author

Please don’t rerereview yet, I finally got around to setting up a windows VM and I want to check if everything works first

@neongreen neongreen force-pushed the emily/jj-xxsmosnkoonr branch from ccae50c to 668bc41 Compare April 19, 2025 12:26
@neongreen neongreen force-pushed the emily/jj-xxsmosnkoonr branch from 668bc41 to 35ad791 Compare April 19, 2025 15:58
@neongreen
Copy link
Copy Markdown
Contributor Author

I’m sorry, I don’t have the energy to work on finishing this in the nearest future. If anyone wants to revisit this later, feel free to use my branch

closing for the same reason

@neongreen neongreen closed this Jul 10, 2025
@glehmann glehmann mentioned this pull request Aug 27, 2025
4 tasks
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.

5 participants