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

Add multiple machines from one project example #605

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

pavelzw
Copy link
Contributor

@pavelzw pavelzw commented Jan 2, 2024

There was still a "real world" example missing from the multiple machine use case.

Also, wouldn't this use case make the target table irrelevant?

The

[dependencies]
cmake = "3.26.4"
python = "3.10"

[target.osx-64.dependencies]
python = "3.11"

example also kind of goes against the philosophy of adding additional requirements to the requirements set (and not replacing them).

IMO the example above should be not solvable since python=3.10, python=3.11 together produce invalid requirements.

Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

I don't think this would remove the target feature completely as you might have small changes in package requirements without it needing a complete new environment that the user has to define in the cli.

If the user wants to get different versions for a package on another target then overwriting still seems valid to me. This is a common case with windows and linux for instance using different cmake versions or completely different build tools.

I would still want to keep this for now as you would need some smarter way of defining a feature to be auto selected. This is where cargos [target.'cfg("platform-name")'] notation would start to make sense.

docs/design_proposals/multi_environment_proposal.md Outdated Show resolved Hide resolved
@pavelzw
Copy link
Contributor Author

pavelzw commented Jan 3, 2024

Alright, then let's leave target be for now. What I personally don't like is that this now introduces two ways of basically doing the same thing.

@pavelzw pavelzw requested a review from ruben-arts January 3, 2024 21:43
Copy link
Contributor

@baszalmstra baszalmstra 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 the example is missing minimal system dependencies!

Nvm.

@ruben-arts ruben-arts merged commit daf63f5 into prefix-dev:main Jan 4, 2024
3 checks passed
@pavelzw pavelzw deleted the multi-env-update branch January 4, 2024 08:46
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.

3 participants