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

Implementation of Pipx Backend #1923

Merged
merged 17 commits into from
Apr 22, 2024
Merged

Implementation of Pipx Backend #1923

merged 17 commits into from
Apr 22, 2024

Conversation

zph
Copy link
Contributor

@zph zph commented Apr 20, 2024

Following from our discussion in Discord about the pipx and GH release backends, here is the pipx one.

This commit adds a pipx backend to mise with the following behaviors:

mise x pipx:psf/black@24.0.3
PIPX_HOME=ctx.tv.install_path PIPX_BIN_DIR=ctx.tv.install_path().join("bin") pipx install git+https://github.com/psf/black@24.0.3

mise x pipx:./psf/black

PIPX_HOME=ctx.tv.install_path PIPX_BIN_DIR=ctx.tv.install_path().join("bin") pipx install ./psf/black

The caveats to this implementation is:

  1. We do not have a mechanism for fetching version and latest version those values are embedded in the project_name

Tasks to accomplish before merging:

  • Alignment on approach
  • Document supported structures of arguments to pipx backend (shorthand for github and longhands)
  • Determine if testing harness needs pipx installed as a dependency (likely yes)
  • Add e2e tests for construction variants of project_names
  • Confirm e2e test behavior compared to path behavior when registered in mise.toml
  • Resolve / remove comments (NOTE/TODO) in PR
  • Decide if there's special uninstall behavior required because of pipx architecture using symlinks. I think not as long as installs are in mise controlled paths.
  • Add documentation to website about pipx backend
  • Align on limitations of version fetching
  • Suppress pipx output about path:
⚠️   Note: '/Users/zph/.local/share/mise/installs/pipx-psf-black/main/bin' is
    not on your PATH environment variable. These apps will not be globally
    accessible until your PATH is updated. Run `pipx ensurepath` to
    automatically add it, or manually modify your PATH in your shell's config
    file (i.e. ~/.bashrc).

I'm posting this for discussion and early feedback and then will do more testing on my own and resolve issues that occur in CI. I had some trouble locally and via docker with testing the e2e pipx test, so interested to see how CI behaves.

Cheers,
ZPH

@zph
Copy link
Contributor Author

zph commented Apr 20, 2024

Ok, I see the test failures and will check them out this weekend.

@zph
Copy link
Contributor Author

zph commented Apr 20, 2024

Unit test failure is a rust security advisory unrelated to my changes:

┌─ /home/runner/work/mise/mise/Cargo.lock:214:1
    │
214 │ rustls 0.21.10 registry+https://github.com/rust-lang/crates.io-index
    │ -------------------------------------------------------------------- security vulnerability detected
    │
    = ID: RUSTSEC-[20](https://github.com/jdx/mise/actions/runs/8761854805/job/24048877053?pr=1923#step:7:21)24-0336

coverage-1 is related to my changes and a failure in test_pipx. I'm debugging why.

@jdx
Copy link
Owner

jdx commented Apr 20, 2024

the unit test failure is because of dependencies with vulnerabilities. I'm going to remove those checks I think but if you pull the latest main it should be fixed.

@zph
Copy link
Contributor Author

zph commented Apr 21, 2024

A conflated problem for the pipx backend is #1926 and I'm going to work around that known behavior for this PR.

zph added 3 commits April 21, 2024 11:51
This commit adds a pipx backend to mise with the following behaviors:

```
mise x pipx:psf/black@24.0.3
PIPX_HOME=ctx.tv.install_path PIPX_BIN_DIR=ctx.tv.install_path().join("bin") pipx install git+https://github.com/psf/black@24.0.3

mise x pipx:./psf/black

PIPX_HOME=ctx.tv.install_path PIPX_BIN_DIR=ctx.tv.install_path().join("bin") pipx install ./psf/black
```

The caveats to this implementation is:
1. We do not have a mechanism for fetching version and latest version
  those values are embedded in the project_name

Tasks to accomplish before merging:
- [ ] Alignment on approac
- [ ] Align on limitations of version fetching
- [ ] Add unit tests for project_name parsing and command building
- [ ] Add e2e tests for construction variants of project_names
@zph
Copy link
Contributor Author

zph commented Apr 22, 2024

@jdx Ok, tests and implementation are in a good spot.

A few outstanding things about PR:

  1. I'm unsure where to add documentation for this backend to show up on mise website. I'm happy to add this in as part of PR if you point me there.
  2. Pipx doesn't have the a backend concept for list_remote_versions and latest_stable_version. I'm stubbing it in as "latest" because implementing the lookups for each respective pipx backend type (git repo, pypi, local project, etc) seems out of scope for this project. Let me know if you have a different suggestion.
  3. Please review how I did the e2e test. I needed to install pipx in order for the e2e to make sense and it assumes python3 is available and builds a temporary venv that it cleans up afterwards. Using virtualenv w/ python3 is the most compatible cross platform install.

@zph
Copy link
Contributor Author

zph commented Apr 22, 2024

When it looks good and is approved, I can squash it to remove the excess commits.

@jdx
Copy link
Owner

jdx commented Apr 22, 2024

while I'm fine starting here, listing remove versions is certainly going to be required functionality as most of what mise does will not function correctly without it so before this comes out of experimental that will need to be in place. btw the docs are located in a separate repo: https://github.com/jdx/mise-docs

nice work!

@jdx jdx merged commit 8b85eaa into jdx:main Apr 22, 2024
7 checks passed
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