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

feat: manifest-path must point to a pixi.toml #324

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

baszalmstra
Copy link
Contributor

@baszalmstra baszalmstra commented Sep 6, 2023

This PR forces the --manifest-path argument to point to a pixi.toml file.

I think its important to always name your project file pixi.toml. This way it's easy for the user to understand what the file is as well as for (future) automation. It also enforces that no two project manifests share the same “.pixi” folder.

@baszalmstra baszalmstra marked this pull request as ready for review September 6, 2023 07:47
@kszlim
Copy link

kszlim commented Sep 6, 2023

Hmm, I'm not sure what prior art is here, I believe cargo does work with alternative manifest names if they're explicitly passed, since pixi is somewhat cargo inspired maybe using similar behaviour here would be fine? The difference with pixi being that you have a shell context where you could potentially just use the environment manifest as default instead of the searched for manifest path.

In my workaround, I'm explicitly using pixi.toml and pixi-cuda.toml so that pixi.toml is the default unless you shell the pixi-cuda.toml manifest.

Do you think supporting the --with-implicit-manifest pixi.toml would be okay as a workaround, I know it bifurcates the modes of operation, but I think having a task runner local to your shell (as opposed to local to your current directory) would have some nice benefits (and would be precursor support multiple global task runners).

Conda is one step short of that in the sense that they provide global envs, but don't have task running support.

@baszalmstra
Copy link
Contributor Author

baszalmstra commented Sep 6, 2023

Cargo has the same behavior:

$ mv Cargo.toml FooBar.toml
$ cargo b --manifest-path FooBar.toml
error: the manifest-path must be a path to a Cargo.toml file

In my workaround, I'm explicitly using pixi.toml and pixi-cuda.toml so that pixi.toml is the default unless you shell the pixi-cuda.toml manifest.

You can still achieve this behavior by placing pixi-cuda.toml in cuda/pixi.toml for instance. However, tasks from that file will only be discovered when you either navigate to that directory or explicitly specify it through the CLI.

Do you think supporting the --with-implicit-manifest pixi.toml would be okay as a workaround.

That would set global state which would alter the behavior of regular pixi commands I don't feel like this is something we should add. For instance, what would the following do:

$ pixi shell --with-implicit-manifest some/random/pixi.toml
$ cd myproject
$ pixi install

Would that then implicitly also use some/random/pixi.toml? As you said, it bifurcates the modes of operation and can lead to surprises.

Conda is one step short of that in the sense that they provide global envs, but don't have task running support.

That's also not what the vision of Pixi is. Different from conda, Pixi offers a project-based workflow, we do not intend to provide global environments in a similar fashion to conda/(micro)mamba. Those tools already do a very good job at that.

@kszlim
Copy link

kszlim commented Sep 6, 2023

ah, sorry, i misremembered. I do think providing that implicit-manifest-path option wouldn't make it confusing, as it would be opt-in behaviour, ie. only people who are explicitly looking for that behaviour would be using it?

@baszalmstra
Copy link
Contributor Author

It's just a foot gun. I can imagine that people will "source" an environment using pixi shell --implicit-manifest-path in their shell profile to be able to emulate a global environment. All other pixi commands will then start behaving differently because basically the isolation between projects has been broken.

@tdejager tdejager merged commit 92b06e5 into prefix-dev:main Sep 7, 2023
9 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.

None yet

3 participants