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

Improve virtual-environment creation #26

Merged
merged 10 commits into from
Dec 30, 2024

Conversation

joshbode
Copy link

@joshbode joshbode commented Dec 23, 2024

  • use poetry run true dummy command to create the virtual-env if it
    doesn't exist (this is necessary for MISE_POETRY_AUTO_INSTALL to
    work)
  • show ANSI colouring when env is created/populated
  • general refactoring

I've tried to interpret what the purpose of the MISE_POETRY_AUTO_VENV is, and I think it's supposed to be an optional guard against "activating" the venv if the lock file is missing.

@joshbode joshbode marked this pull request as draft December 23, 2024 05:51
@joshbode joshbode force-pushed the joshbode/improve-venv-creation branch 3 times, most recently from 3c78761 to bf141ab Compare December 23, 2024 06:14
@joshbode joshbode marked this pull request as ready for review December 23, 2024 06:15
@joshbode joshbode force-pushed the joshbode/improve-venv-creation branch 3 times, most recently from f6a7436 to 00f3920 Compare December 23, 2024 14:43
Copy link
Member

@jdx jdx left a comment

Choose a reason for hiding this comment

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

I think we need some test cases for this logic. I would look at the e2e tests in mise like https://github.com/jdx/mise/blob/main/e2e/core/test_python_venv for some inspiration.

bin/exec-env Outdated

if is_enabled MISE_POETRY_AUTO_INSTALL; then
echoerr "Executing \`poetry install\` to populate virtual environment."
poetry_bin "${project_root}" --ansi install
Copy link
Member

Choose a reason for hiding this comment

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

the terminal might not support colors. I think there is an env var that gets passed from mise saying if there are colors enabled. If not we should add that first to detect if we can safely enable colors.

Copy link
Author

Choose a reason for hiding this comment

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

there's MISE_COLOR but I'm not sure it gets passed into plugins - is that the one you were thinking of?
There is also reference to CLICOLOR in the docs, but I don't think that gets passed in either.

poetry uses the NO_COLOR env var for the same purpose, I think.

The reason I had set --ansi because when poetry runs non-interactively in the plugin it doesn't display colours.

Copy link
Member

Choose a reason for hiding this comment

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

there are situations like if TERM=dumb where it can be a tty but colors should still not be enabled. I think we should add a new env var like MISE_PLUGIN_COLORS=1 to this list: https://github.com/jdx/mise/blob/ba66d4659c6d8f3ffa589dacfe402d6988e46d9a/src/plugins/asdf_plugin.rs#L410-L416

we don't want to use MISE_COLOR I don't think because mise might be used for non-tty things inside of a plugin

Copy link
Author

Choose a reason for hiding this comment

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

OK - I've added a check on (the currently non-existent) MISE_PLUGIN_COLORS which means that --ansi is disabled for now

- use `poetry run true` dummy command to create the virtual-env if it
  doesn't exist (this is necessary for `MISE_POETRY_AUTO_INSTALL` to
  work)
- show ANSI colouring when env is created/populated
- general refactoring
@joshbode joshbode force-pushed the joshbode/improve-venv-creation branch from 00f3920 to 138ceb3 Compare December 23, 2024 22:22
@joshbode
Copy link
Author

I think we need some test cases for this logic. I would look at the e2e tests in mise like https://github.com/jdx/mise/blob/main/e2e/core/test_python_venv for some inspiration.

I'm not done yet (needs more tests), but I made a start in tests by scripting mise exec -C "${dir}" -- to "activate" the environments in isolated projects with different configurations.

@joshbode joshbode force-pushed the joshbode/improve-venv-creation branch from 008da6b to aedc98b Compare December 24, 2024 02:23
@joshbode joshbode force-pushed the joshbode/improve-venv-creation branch from 8aceea5 to 017d25c Compare December 24, 2024 02:34
@joshbode joshbode force-pushed the joshbode/improve-venv-creation branch 2 times, most recently from be42ef2 to feed16a Compare December 24, 2024 09:47
@joshbode joshbode force-pushed the joshbode/improve-venv-creation branch from feed16a to 11dd1bd Compare December 24, 2024 09:50
@joshbode
Copy link
Author

joshbode commented Dec 24, 2024

I tried to get the tests to run as a mise run task, but it didn't seem to mix well with the mise exec strategy, even though it runs nicely locally 🤷

@jdx
Copy link
Member

jdx commented Dec 30, 2024

wow nice work! is it ready?

@joshbode
Copy link
Author

wow nice work! is it ready?

Thanks @jdx :)

I think it is - I'm just not sure whether I interpreted the MISE_POETRY_AUTO_VENV feature correctly in my refactor, but at least there's a test case to discuss now.

@jdx jdx merged commit 0bb2c1c into mise-plugins:main Dec 30, 2024
4 checks passed
@joshbode joshbode deleted the joshbode/improve-venv-creation branch December 30, 2024 11:22
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