mise icon indicating copy to clipboard operation
mise copied to clipboard

feat: Proof of concept: Use ubi as a library instead of as a binary

Open autarch opened this issue 1 year ago • 12 comments

I took a stab at this using the library-ize branch of ubi. It seems to work from my testing locally, and the test suite also passed locally.

autarch avatar Jun 15 '24 04:06 autarch

I honestly don't understand the internals of mise well enough to know if this PR is done or not.

autarch avatar Jun 15 '24 04:06 autarch

it seems to be failing but it's unclear why:

E2E ./backend/test_ubi_token
  $ GITHUB_TOKEN=foobar mise install -f ubi:goreleaser/[email protected] 2>&1
  Error: [GITHUB_TOKEN=foobar mise install -f ubi:goreleaser/[email protected] 2>&1] command failed with status 1
  ./backend/test_ubi_token: 0s
  Error: exited with status code 1
  Test environment can be examined in /tmp/test_ubi_token.JQmoym
mise [test:coverage] exited with code 1
Error: Final attempt failed. Child_process exited with error code 1

jdx avatar Jun 15 '24 12:06 jdx

it seems to be failing but it's unclear why:

E2E ./backend/test_ubi_token
  $ GITHUB_TOKEN=foobar mise install -f ubi:goreleaser/[email protected] 2>&1
  Error: [GITHUB_TOKEN=foobar mise install -f ubi:goreleaser/[email protected] 2>&1] command failed with status 1
  ./backend/test_ubi_token: 0s
  Error: exited with status code 1
  Test environment can be examined in /tmp/test_ubi_token.JQmoym
mise [test:coverage] exited with code 1
Error: Final attempt failed. Child_process exited with error code 1

This test replaces the ubi binary with a shell script that echoes back the GitHub token. It's testing that various env vars which contain a token are ultimately passed to ubi. I'm not sure exactly how to test this with ubi as a library.

autarch avatar Jun 15 '24 14:06 autarch

Also, I think it'd be nice to add support for some other features, like telling ubi the executable name or telling it that the release it picks must match a certain string. The library-ized code supports this, but it's not clear how this would be done with mise. Would it make sense to add some additional ubi-specific flags, something like:

mise "ubi:houseabsolute/[email protected]" --ubi-matching musl --ubi-exe precious

The other options I can think of are:

  • Extend the syntax for packages that mise accepts to allow for more stuff. But that seems really gross and confusing. I don't think allowing per-backend extensions in that string would make for a nice UI.
  • Make mise look for env vars like UBI_MATCHING instead of flags. Personally, I think flags are better since they're self-documenting when you use clap. But on the flip side, adding lots of flags for each different backend could become quite unruly.

autarch avatar Jun 15 '24 14:06 autarch

mise has support for tool config which should be documented more clearly:

[tools]
# send arbitrary options to the plugin, passed as:
# MISE_TOOL_OPTS__VENV=.venv
python = { version = '3.10', virtualenv = '.venv' }

which could be leveraged here. What's missing is a way to configure this on the command line in part because it's tricky to know how to be able to do that with multiple tools

jdx avatar Jun 19 '24 13:06 jdx

mise has support for tool config which should be documented more clearly:

[tools]
# send arbitrary options to the plugin, passed as:
# MISE_TOOL_OPTS__VENV=.venv
python = { version = '3.10', virtualenv = '.venv' }

which could be leveraged here. What's missing is a way to configure this on the command line in part because it's tricky to know how to be able to do that with multiple tools

I pushed a commit that adds the ability to set "exe" and "matching" in the config file. I think it's ok if this is only available via the config file for now.

autarch avatar Jun 23 '24 08:06 autarch

Hello not sure if linked but i have some issue with existing ubi for now like https://github.com/jdx/mise/issues/2429

yodatak avatar Aug 14 '24 02:08 yodatak

@autarch let me know when you think this is in a good place for review (or if it's ready now)

jdx avatar Aug 16 '24 15:08 jdx

@jdx I think this is mostly ready, but it wouldn't hurt to add some additional e2e tests, I think.

autarch avatar Aug 25 '24 17:08 autarch

Also, there's an e2e test failure I don't understand - https://github.com/jdx/mise/actions/runs/10548662451/job/29222829250?pr=2290

autarch avatar Aug 25 '24 17:08 autarch

I think we can remove that e2e test file entirely since it involves mocking out ubi which isn't possible anymore baking the CLI into the codebase. It's running this which fails as you would expect:

mise  ubi-as-library ❯ GITHUB_TOKEN=foobar m i -f ubi:goreleaser/[email protected]
Error: 
   0: HTTP status client error (401 Unauthorized) for url (https://api.github.com/repos/goreleaser/goreleaser/releases/tags/v1.25.0)

Location:
   src/backend/ubi.rs:85

Version:
   2024.8.12-DEBUG macos-arm64 (23eecad 2024-08-27)

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

jdx avatar Aug 27 '24 22:08 jdx

there's a couple more issues I'd like to see tackled before we include this, generally to avoid adding a ton of crates to increase the build complexity of mise further than it already is:

  • https://github.com/houseabsolute/ubi/issues/62 - this one is more important since it would greatly reduce crate count while also adding some flexibility, it should just be a few lines in Cargo.toml but I'm happy to help if you run into any problems since I've dealt with this quite a bit
  • https://github.com/houseabsolute/ubi/issues/61 - this one is less important, there is one crate which is out of date but the idea here is this would keep them current in the future—mitigating the # of outdated and therefore duplicated crates in our CLIs

jdx avatar Aug 27 '24 22:08 jdx

I had a tiny nit but I think this is ready. This is pretty exciting since we should be able to remove a bunch of asdf plugins and just use ubi as the backend!

jdx avatar Oct 13 '24 11:10 jdx

I just merged from your main and resolved the conflicts.

autarch avatar Oct 13 '24 14:10 autarch

But now I'm getting a very confusing compilation error:

cargo test --workspace
   Compiling mise v2024.10.2 (/home/autarch/projects/mise)
error[E0277]: the trait bound `toml_edit::Item: From<toml_edit::Value>` is not satisfied
  --> src/cli/settings/set.rs:64:42
   |
64 |         parent_table.insert(child, value.into());
   |                                          ^^^^ the trait `From<toml_edit::Value>` is not implemented for `toml_edit::Item`, which is required by `toml_edit::Value: Into<_>`
   |
   = note: required for `toml_edit::Value` to implement `Into<toml_edit::Item>`

error[E0277]: the trait bound `toml_edit::Item: From<toml_edit::Value>` is not satisfied
  --> src/cli/settings/set.rs:74:36
   |
74 |         settings.insert(key, value.into());
   |                                    ^^^^ the trait `From<toml_edit::Value>` is not implemented for `toml_edit::Item`, which is required by `toml_edit::Value: Into<_>`
   |
   = note: required for `toml_edit::Value` to implement `Into<toml_edit::Item>`

git log -p For more information about this error, try `rustc --explain E0277`.
error: could not compile `mise` (bin "mise") due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `mise` (bin "mise" test) due to 2 previous errors

No idea what's going on here.

autarch avatar Oct 13 '24 14:10 autarch

Thanks for all the hard work that went into getting UBI built-in within mise :)

I've now switched most things that can be installed with UBI over to using it in my own global config: https://github.com/jimeh/dotfiles/commit/5f0330dcafc60e99a38163b09e3a48e4b981b0b8

jimeh avatar Oct 15 '24 22:10 jimeh

Thanks @jimeh for your config, I was looking for a quick way to change asdf to ubi

sirenkovladd avatar Oct 15 '24 23:10 sirenkovladd

@jimeh if you've verified these work we should probably replace the shorthands in registry.toml to use ubi by default

jdx avatar Oct 15 '24 23:10 jdx

@jdx do you have any minimum os/arch set of binaries provided in GitHub releases in mind before making ubi the default for a specific tool? Or does it maybe not matter, as most asdf plugins which ubi can replace are probably just downloading binaries from GitHub releases already?

As for those in my config, I haven't tried executing all of them, but they all installed fine :)

I only double-checked about 60% of the asdf plugins I switched away from to confirm that all they do is download GitHub release assets, as I just went straight to the relevant repo and looked at the most recent release for projects I'm quite familiar with. I'd be happy to go back and check them all though.

All the asdf plugins left in my config download the binaries from somewhere other than GitHub releases. The semi-exception is eza, which if I understood it correctly, supports falling back to cargo-quickinstall's pre-built binaries if the official repo doesn't have a binary for your os/arch.

jimeh avatar Oct 16 '24 00:10 jimeh

@jdx do you have any minimum os/arch set of binaries provided in GitHub releases in mind before making ubi the default for a specific tool? Or does it maybe not matter, as most asdf plugins which ubi can replace are probably just downloading binaries from GitHub releases already?

yeah since we'd be replacing the asdf plugin the arch/os support will be identical with ubi

I only double-checked about 60% of the asdf plugins I switched away from to confirm that all they do is download GitHub release assets, as I just went straight to the relevant repo and looked at the most recent release for projects I'm quite familiar with. I'd be happy to go back and check them all though.

I think anything you're welcome to contribute here would be helpful, it'd definitely be nice to simplify and remove the asdf plugin where we can.

All the asdf plugins left in my config download the binaries from somewhere other than GitHub releases. The semi-exception is eza, which if I understood it correctly, supports falling back to cargo-quickinstall's pre-built binaries if the official repo doesn't have a binary for your os/arch.

We actually have support for this because the registry supports multiple backends, e.g.:

eza = ['ubi:eza/eza', 'cargo:eza']

People on esoteric architectures could then set MISE_DISABLE_BACKENDS=ubi if they want which would make mise default to using the next item, so in this case cargo.

jdx avatar Oct 16 '24 01:10 jdx

That all makes sense. In that case I'll do a PR when I'm back on my computer :)

Do you prefer separate PRs for each tool against registry.toml, or happy with a single PR that updates a bunch of tools?

jimeh avatar Oct 16 '24 08:10 jimeh

oh just make 1

jdx avatar Oct 16 '24 12:10 jdx

I've held off on the registry.toml PR for now cause I have run into some issues with some of the tools I swapped over to use ubi as new releases were published. Hence I plan to have a bit of a closer look at them, GH releases, assets, and what the asdf plugin do to properly verify the ones I switch in a registry.toml PR are unlikely to have any issues :)

I also ran into issues with 403 errors from GitHub's API, which has led me to expose a GitHub token as an env var again, instead of having tools query it out of 1Password's CLI when they need it as I've been doing recently. I'll look at some other issues mentioning this when I have time, and see if I provide any useful suggestions.

jimeh avatar Oct 21 '24 17:10 jimeh