-
Notifications
You must be signed in to change notification settings - Fork 392
feat(add): --channel parameter to specify channel.
#4987
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Pradyot Ranjan <[email protected]>
|
Thanks for the contribution. Could you take a look at the CI issues? Can you also add tests? Especially around what happens if the channel is already present? There is also syntax for matchspec, which looks like |
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
I'm quite new to pixi 😅 could you elaborate on this a bit more? |
Signed-off-by: Pradyot Ranjan <[email protected]>
… into add-channel-parameter
|
@baszalmstra I've tried adding support of MatchSpec's channel syntax. Users can now do |
|
Something seems to have broken in the tests. Can you take a look? I will hold off with merging this until @ruben-arts gets back. I want his approval with regard to the UX. |
Signed-off-by: prady0t <[email protected]>
Signed-off-by: prady0t <[email protected]>
Signed-off-by: prady0t <[email protected]>
crates/pixi_cli/src/add.rs
Outdated
|
|
||
| // Add a channel | ||
| let channel = args.channel; | ||
| workspace.manifest().add_channels([PrioritizedChannel::from(NamedChannelOrUrl::Name(String::from(channel.unwrap_or_default()))).clone()], &FeatureName::DEFAULT, false)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line just calls the add_channels() function with the args given from the user, so testing this is essentially testing the functionalities of add_channel() which is already tested and also includes the situation where we try to add channel when it's already present.
@baszalmstra Do you think we sould still try to add tests for this? If so could me point to where the tests for cli inputs are(or should be) present.
| # successful after adding the channel | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "add", | ||
| "https://repo.prefix.dev/bioconda::snakemake-minimal", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| stderr_contains="Added https://repo.prefix.dev/bioconda::snakemake-minimal", | ||
| ) | ||
|
|
||
| # no message for initially unused feature... | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "add", | ||
| "https://conda.anaconda.org/conda-forge::xz", | ||
| "--feature=prefix", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| ) | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "workspace", | ||
| "environment", | ||
| "add", | ||
| "prefix", | ||
| "--feature=prefix", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| ) | ||
| # ...but decent message on install: | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "install", | ||
| "--environment=prefix", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| expected_exit_code=ExitCode.FAILURE, | ||
| stderr_contains="unavailable channel 'https://conda.anaconda.org/conda-forge/'", | ||
| ) | ||
| # and helpful message now feature is used: | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "add", | ||
| "https://conda.anaconda.org/conda-forge::libzlib", | ||
| "--feature=prefix", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| expected_exit_code=ExitCode.FAILURE, | ||
| stderr_contains="pixi workspace channel add https://conda.anaconda.org/conda-forge", | ||
| ) | ||
|
|
||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "workspace", | ||
| "channel", | ||
| "add", | ||
| "--feature=prefix", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these as using matchspec syntax adds the channel if not found, hence these tests are irrelevant.
| spec_type: SpecType, | ||
| dep_options: DependencyOptions, | ||
| git_options: GitOptions, | ||
| channel: Option<Vec<NamedChannelOrUrl>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
channel is now Option<Vec> as oppose to Option earlier. This helps us take url and local channel paths as part of matchspec as well.
Signed-off-by: prady0t <[email protected]>
|
The tests are now passing, and I've added some tests to check for |
| "workspace", | ||
| "channel", | ||
| "add", | ||
| "https://repo.prefix.dev/bioconda", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| ) | ||
| # successful after adding the channel | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "add", | ||
| "https://repo.prefix.dev/bioconda::snakemake-minimal", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| stderr_contains="Added https://repo.prefix.dev/bioconda::snakemake-minimal", | ||
| ) | ||
|
|
||
| # no message for initially unused feature... | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "add", | ||
| "https://conda.anaconda.org/conda-forge::xz", | ||
| "--feature=prefix", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| ) | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "workspace", | ||
| "environment", | ||
| "add", | ||
| "prefix", | ||
| "--feature=prefix", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| ) | ||
| # ...but decent message on install: | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "install", | ||
| "--environment=prefix", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| expected_exit_code=ExitCode.FAILURE, | ||
| stderr_contains="unavailable channel 'https://conda.anaconda.org/conda-forge/'", | ||
| ) | ||
| # and helpful message now feature is used: | ||
| verify_cli_command( | ||
| [ | ||
| pixi, | ||
| "add", | ||
| "https://conda.anaconda.org/conda-forge::libzlib", | ||
| "--feature=prefix", | ||
| "--manifest-path", | ||
| tmp_pixi_workspace, | ||
| ], | ||
| expected_exit_code=ExitCode.FAILURE, | ||
| stderr_contains="pixi workspace channel add https://conda.anaconda.org/conda-forge", | ||
| ) | ||
|
|
||
| verify_cli_command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove these tests?
|
My question is whether we should force the dependencies you specify when calling them with pixi add --channel bioconda snakemake samtools[workspace]
channels = ["conda-forge", "bioconda"]
platforms = ["osx-arm64"]
[dependencies]
snakemake = ">=9.14.3,<10"
samtools = ">=1.22.1,<2"But Creates: [workspace]
channels = ["conda-forge", "https://conda.anaconda.org/bioconda"] # I would say this conda.anaconda.org part can be seen as a bug...
platforms = ["osx-arm64"]
[dependencies]
snakemake = { version = ">=9.14.3,<10", channel = "bioconda" } # Specifically this channel
samtools = { version = ">=1.22.1,<2", channel = "bioconda" } # Specifically this channelShould these two-way's of defining create the same TOML? I think yes. |
|
I agree. @prady0t You can use the |
|
Thanks for the review, this is a bug indeed. Working on it now. |
Description
Fixes #4594
How Has This Been Tested?
Building locally and running pixi add --channel [channel_name] [package] now works and the manifest files are updated accordingly. Test it out with
pixi add --channel bioconda samtoolsChecklist:
schema/model.py.