fix handling of `--all-groups` and `--no-default-groups` flags by Gankra · Pull Request #11224 · astral-sh/uv

Gankra · 2025-02-04T19:28:17Z

This is a rewrite of the groups subsystem to have more clear semantics, and some adjustments to the CLI flag constraints. In doing so, the following bugs are fixed:

--no-default-groups --no-group foo is no longer needlessly rejected
--all-groups --no-default-groups now correctly evaluates to --all-groups where previously it was erroneously being interpretted as just --no-default-groups
--all-groups --only-dev is now illegal, where previously it was accepted and mishandled, as if it was a mythical --only-all-groups flag

Fixes #10890
Closes #10891

unambiguous semantics and a clearer implementation.

zanieb · 2025-02-04T20:50:30Z

crates/uv-cli/src/lib.rs

+    #[arg(long, conflicts_with_all = ["group", "dev", "all_groups"])]
    pub only_dev: bool,


--only-dev and --dev do not conflict, --no-dev?

Suggested change

#[arg(long, conflicts_with_all = ["group", "dev", "all_groups"])]

pub only_dev: bool,

#[arg(long, conflicts_with_all = ["group", "no_dev", "all_groups"])]

pub only_dev: bool,

The idea I'm trying to express here is essentially that --dev --only-dev is "what are you even doing".

This is more clear with --group A --only-group B, which is clearly "what no", but rejecting that and not rejecting --group A --only-group A is a pain for very little benefit. Since --dev --only-dev essentially desugars to that, I figured it's best to reject it for uniformity (and the followup code essentially assumes only one of these flags is ever true, although in a trivial way where we could fix it to "saturate" to --only-dev).

That makes sense. I'd prefer to just allow redundant options, but understand it can get complicated and if it's not worth it it's not a big deal. Is it a breaking change though?

--only-dev --no-dev should still error though, no? Or does it not because we say that "exclusions override" and that's fine?

Yeah I wasn't sure how exclusions and overrides interact. It should probably be tightened up but I didn't want to break something (I'll test it out / read the docs properly to figure out how this should be handled).

In terms of breaking... hmm... seems like the old impl tried harder to rationalize both inputs being provided... but in a bit of an adhoc way...

uv/crates/uv-configuration/src/dev.rs

Lines 234 to 256 in ac10042

if matches!(dev, Some(DevMode::Only)) {

unreachable!("cannot specify both `--only-dev` and `--group`")

};

// Ensure that `--no-group` and `--group` are mutually exclusive.

group.retain(|group| !no_group.contains(group));

Some(GroupsSpecification::Include {

include: IncludeGroups::Some(group),

exclude: no_group,

})

} else if !only_group.is_empty() {

if matches!(dev, Some(DevMode::Include)) {

unreachable!("cannot specify both `--dev` and `--only-group`")

};

// Ensure that `--no-group` and `--only-group` are mutually exclusive.

only_group.retain(|group| !no_group.contains(group));

Some(GroupsSpecification::Only {

include: only_group,

exclude: no_group,

})

In general, I'd say a breaking change here would not be ideal. I'd suggest PR'ing some tests for edge-cases to main first so we can see if they change here? If it seems hard to avoid a regression, then I guess we should wait for 0.6?

Ok cool so this is the Final Potential Breakage to hash out (collapsed a few others that I regard as Equivalent).

Looking closer, the current code:

Hard panics(!) on --group --only-dev and --dev --only-group

CLI conflicts --group --only-group

CLI conflicts --dev --only-dev

Saturates --only-dev --dev to --only-dev

CLI conflicts --only-dev --no-dev

Positionally Overrides --dev --no-dev --dev ...

So overall the current code hard rejects trying to mix --only-* with --group/--dev, except in the exact case of --only-dev --dev, which it turns into --only-dev. Which is fair enough, if a bit inconsistent. It's interesting that there's no overriding between --only-dev and --no-dev but I don't want to poke that bear in this PR.

I've adjusted things to preserve this behaviour, although the hard panics are now CLI conflicts.

crates/uv-cli/src/lib.rs

crates/uv-configuration/src/dev.rs

crates/uv/src/commands/project/mod.rs

crates/uv-configuration/src/dev.rs

zanieb · 2025-02-04T23:54:29Z

crates/uv/src/commands/project/run.rs

                },
                &ExtrasSpecification::default(),
-                &DevGroupsManifest::default(),
+                &DevGroupsSpecification::default().with_defaults(Vec::new()),


This looks a little weird?

Related https://github.com/astral-sh/uv/pull/11224/files#r1942059001

I agree it's a bit weird, but on the other hand I do kind of like that it's really hammering in "there were two different things I was theoretically supposed to be inputting and I'm doing Neither of them".

crates/uv-configuration/src/dev.rs

zanieb · 2025-02-05T20:08:25Z

crates/uv/tests/it/sync.rs

+    // --dev --only-dev should saturate as --only-dev
+    uv_snapshot!(context.filters(), context.sync()


Weird to allow this but not // --group and --only-group should error if they name same things but... eh.

Preserving previous behaviour :)

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.27` -> `0.5.29` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.5.29`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0529) [Compare Source](astral-sh/uv@0.5.28...0.5.29) ##### Enhancements - Add `--bare` option to `uv init` ([#11192](astral-sh/uv#11192)) - Add support for respecting `VIRTUAL_ENV` in project commands via `--active` ([#11189](astral-sh/uv#11189)) - Allow the project `VIRTUAL_ENV` warning to be silenced with `--no-active` ([#11251](astral-sh/uv#11251)) ##### Python The managed Python distributions have been updated, including: - CPython 3.12.9 - CPython 3.13.2 - pkg-config files are now relocatable See the [`python-build-standalone` release notes](https://github.com/astral-sh/python-build-standalone/releases/tag/20250205) for more details. ##### Bug fixes - Always use base Python discovery logic for cached environments ([#11254](astral-sh/uv#11254)) - Use a flock to avoid concurrent initialization of project environments ([#11259](astral-sh/uv#11259)) - Fix handling of `--all-groups` and `--no-default-groups` flags ([#11224](astral-sh/uv#11224)) ##### Documentation - Minor touchups to the Docker provenance docs ([#11252](astral-sh/uv#11252)) - Move content from the `mkdocs.public.yml` into the template ([#11246](astral-sh/uv#11246)) ### [`v0.5.28`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0528) [Compare Source](astral-sh/uv@0.5.27...0.5.28) ##### Bug fixes - Allow discovering virtual environments from the first interpreter found on the `PATH` ([#11218](astral-sh/uv#11218)) - Clear ephemeral overlays when running tools ([#11141](astral-sh/uv#11141)) - Disable SSL in Git commands for `--allow-insecure-host` ([#11210](astral-sh/uv#11210)) - Fix hardlinks in tar unpacking ([#11221](astral-sh/uv#11221)) - Set base executable when returning virtual environment ([#11209](astral-sh/uv#11209)) - Use base Python for cached environments ([#11208](astral-sh/uv#11208)) ##### Documentation - Add documentation on verifying Docker image attestations ([#11140](astral-sh/uv#11140)) - Add `last updated` to documentation ([#11164](astral-sh/uv#11164)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Rewrite DevGroupSpecification and DevGroupManifest to have

aad2175

unambiguous semantics and a clearer implementation.

Gankra added the internal A refactor or improvement that is not user-facing label Feb 4, 2025

Gankra added 5 commits February 4, 2025 14:39

remove some dead code

b67eb09

wrap in an Arc

ee1fabb

regen snapshots

b3252c4

rejig the conflicts on dependency group flags

50aa278

cleanup with_default impl

1c75dd0