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

upgrade --tighten #3957

Merged
merged 4 commits into from
Sep 1, 2023
Merged

upgrade --tighten #3957

merged 4 commits into from
Sep 1, 2023

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Jun 27, 2023

Used to programmatically tighten lower bounds on dependencies in pubspec.yaml

Motivation

For a Dart project pubspec.yaml gives the version constraints of a project's dependencies. And pubspec.lock contains the current resolution of concrete versions to fulfill those constraints.
Currently the command dart pub upgrade can help the user upgrade the version of a dependency that a user is locked on in pubspec.lock within the current constraint from pubspec.yaml.
dart pub upgrade --major-versions can help upgrade the constraint to bump the constraint to a later version range. It will also upgrade the lower bound, but only of those packages that needed a breaking change.
When you are authoring a package for others to consume, it makes sense to have as wide constraints as possible to minimize version conflicts. To test compatibility with lower bounds one can use dart pub downgrade to get a resolution with as low versions as possible.
However, for some uses, for example when making an app, it makes little sense to keep the support for older versions of dependencies. Therefore we propose dart pub upgrade --tighten as a way to bump the constraints to match the versions actually resolved.
A feature like this has been requested by the community.

Example

Here is an example transcript (slightly simplified for clarity):

$ cat pubspec.yaml
name: sample
environment:
  sdk: ^3.0.0
dependencies:
  dio: ^4.0.0

$ dart pub get
Resolving dependencies... 
  dio 4.0.1 (5.3.2 available) # Already locked to 4.0.1

$ dart pub upgrade --dry-run # Current upgrade only touches lock-file
> dio 4.0.6 (was 4.0.1) (5.3.2 available)
Would change 1 dependency.

$ dart pub upgrade --tighten --dry-run
> dio 4.0.6 (was 4.0.1) (5.3.2 available)
Would change 1 dependency.
Would change 1 constraint in pubspec.yaml:
  dio: ^4.0.1 -> ^4.0.6

$ dart pub upgrade --tighten --major-versions # Allows breaking version changes
+ async 2.11.0
> dio 5.3.2 (was 4.0.1)
+ meta 1.9.1
Changed 3 dependencies!

Changed 1 constraint in pubspec.yaml:
  dio: ^4.0.1 -> ^5.3.2

$ cat pubspec.yaml
name: sample
environment:
  sdk: ^3.0.0
dependencies:
  dio: ^5.3.2

Details:

any constraints:

A hosted dependency that is currently constrained to any version (this is the default if no constraint is given) will be modified to be constrained to ^current. (That is, they will also get an upper bound).
A non-hosted dependency with no constraints will not be modified by --tighten.

Dry run

When run with --dry-run a list of proposed changes will be written to the terminal.
Combined with --major-versions
dart pub --major-versions --tighten will result in the same resolutions as dart pub --major-versions, but all dependencies will have their constraint lower bound tightened, not only those that have their upper bound modified.

Upgrading only selected packages:

The upgrade --tighten and upgrade --tighten --major-versions can be combined with one or more package-names. In that case only those packages are upgraded. (Just like the current upgrade and upgrade --major-versions.)

Alternative names:

Several names have been considered

  • --atleast-locked
  • --tight-lower-bounds

After this has landed, we need to:

TODO(sigurdm): roll to SDK
TODO(sigurdm): Documentation section on https://dart.dev/tools/pub/cmd/pub-upgrade
TODO(sigurdm): ensure the --tighten argument is passed through flutter pub upgrade (add argument to allowlist here).

Fixes: #3924

@sigurdm sigurdm requested a review from jonasfj June 27, 2023 14:01
Copy link
Member

@jonasfj jonasfj 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 this is a good idea. But let's make sure we all agree we want to do this.

'Running `upgrade --tighten` only in `${entrypoint.rootDir}`. Run `$topLevelProgram pub upgrade --tighten --directory example/` separately.',
);
}
final toTighten = argResults.rest.isEmpty
Copy link
Member

Choose a reason for hiding this comment

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

nit: Maybe we should have a getter:

/// List of package names to upgrade, if empty then upgrade all packages.
///
/// This allows the user to specify list of names that they want the
/// upgrade command to affect.
List<String> get packagesToUpgrade => argResults.rest;

We could also call it targetPackages, just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 177 to 181
/// If packages to update where given on the commandline, only those are
/// tightened. Otherwise all packages are tightened.
///
/// If dependency has already been updated in [changes], the update will apply
/// on top of that change.
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this just be a function taking a List<PackageRange> and returning a List<PackageRange>.

Then maybe _updatePubspec(List<PackageRange> target) and it has to figure out on its own which have changed and which haven't.

Ofcourse, I see that if we wanted to make those changes we should really make the refactoring to _updatePubspec in a separate PR (and land it first).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is convenient in _outputChangeSummary that we have a mapping from original to new package range.

I could return an updated mapping instead of modifying teh existing map.

Done

@parlough
Copy link
Member

parlough commented Aug 28, 2023

I love this!

I think this is a good idea. But let's make sure we all agree we want to do this.

Since you're looking for some affirmation, I think it's a great idea. I do this occasionally in a few of my workflows and have seen others do it. As for the name, "tighten" is the verb I've used in related commit messages before, and it seems to have been well understood by reviewers.

@sigurdm sigurdm merged commit 1ff7ed9 into dart-lang:master Sep 1, 2023
23 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.

Add a way to tighten the lower bounds in pubspec file.
3 participants