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

use pub workspaces #2249

Merged
merged 6 commits into from
Jul 8, 2024
Merged

use pub workspaces #2249

merged 6 commits into from
Jul 8, 2024

Conversation

jakemac53
Copy link
Contributor

There is one weird quirk here - our usual way of pinning package versions doesn't work, because we are pinning a version which doesn't yet exist, using the non-wip number. I resolved that by adding version constraints like this instead >=x.x.x-wip <x.x.(x+1). But, it is a bit weird. When creating a release, we can change these to exact versions, because they will then match the version in the repo. But, we might not remember to. WDYT @natebosch ?

Also cc @sigurdm and @jonasfj in case they have a different solution to suggest.

@jonasfj
Copy link
Member

jonasfj commented Jul 2, 2024

  • Don't pin versions 🤣
  • If there are reasons for pinning (reconsider them, like could you just merge tightly coupled packages into a single package).
  • If that fails, maybe reconsider -wip, like is that a good convention?
  • If that doesn't work, consider using dependency_overrides to avoid problems with version constraints. Of course this means constraints won't be checked.
  • If that unacceptable: the consider accepting that maybe this doesn't have a good solution 🙈

I don't necessarily have an answer here. But I think it's worthwhile questioning some of the decisions that led us here.

If we want to check version constraints in workspace (and we do) using a -wip is going to be a problem.
It's also an annoying convention. It might be possible to do smarter things, like maybe keep a WIP entry in the CHANGELOG, and only update version number there when publishing...

Or we could use GitHub Actions to bump version numbers and publish using workflow_dispatch. If we wanted to automate some of the tedious manual bits.

I don't have an answer off the top of my head, I'm just not 100% sure we shouldn't reconsider our workflow / conventions around -wip.

@natebosch
Copy link
Member

  • Don't pin versions 🤣

It's not clear how much effort that would take. We made a lot of progress in decoupling these packages, but not enough to use a looser dependency yet.

  • like could you just merge tightly coupled packages into a single package

We split the packages to allow some use cases to have pruned transitive deps, so we can't merge them without changes in pub.

  • maybe reconsider -wip, like is that a good convention?

It's our documented convention. https://github.com/dart-lang/sdk/blob/main/docs/External-Package-Maintenance.md#making-a-change

I find it's convenient to have a quick place to check whether there are unpublished changes, and reliable enough even without enforcement. Without this we have more often made mistakes like double-incrementing the version number with a single publish.

If we adopt tooling which does the edits for us we could implement it to check for published versions and warn about double incrementing maybe. I'm uneasy about dropping the practice without tooling though.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Jul 2, 2024

  • Don't pin versions 🤣

We might consider this at this point, but it wouldn't solve the problem. Any time we have a new feature in one package, that isn't released yet, we will have to use one of these -wip constraints.

  • If there are reasons for pinning (reconsider them, like could you just merge tightly coupled packages into a single package).

It used to be one giant package, but was split out for good reasons (fewer transitive deps). Flutter only depends on the smaller (test_api) package for the most part (at least in terms of non-dev dependencies), which means a lot fewer pinned deps in total.

  • If that fails, maybe reconsider -wip, like is that a good convention?

Yes, it is a good convention, and it is the general established convention for dart-lang packages.

Packages are never published with that version, it just indicates "there are unpublished changes, and based on those this is the next anticipated version number". It makes it easy to tell at a glance which packages have unpublished changes and also allows you to evolve the next pending release version along with the changes actually being made.

  • If that doesn't work, consider using dependency_overrides to avoid problems with version constraints. Of course this means constraints won't be checked.

Workspaces don't allow dependency_overrides of the packages in the workspace (this is what we previously did).

  • If that unacceptable: the consider accepting that maybe this doesn't have a good solution 🙈

Yeah, the solution I came up with is sort of ok, not ideal but works. I prefer it to dependency overrides, because we can at least ensure our version constraints are compatible. We have before forgotten to bump pinned versions because of the overrides.

I don't necessarily have an answer here. But I think it's worthwhile questioning some of the decisions that led us here.

If we want to check version constraints in workspace (and we do) using a -wip is going to be a problem. It's also an annoying convention. It might be possible to do smarter things, like maybe keep a WIP entry in the CHANGELOG, and only update version number there when publishing...

We could do that but it feels wrong to have the changelog/pubspec out of sync. It is a work in progress version, and so having that in the pubspec seems correct to me. It is also error prone to not do that. We want to be able to update the pubspec in other workspace packages to depend on WIP features (see last comment below).

Or we could use GitHub Actions to bump version numbers and publish using workflow_dispatch. If we wanted to automate some of the tedious manual bits.

Maybe, not sure exactly what this would look like.

I don't have an answer off the top of my head, I'm just not 100% sure we shouldn't reconsider our workflow / conventions around -wip.

Yeah I am not suggesting changing how the version solve works, or hard coding knowledge about -wip. But I do think it is worth considering even just the "new feature" use case.

  • You are developing a feature that stretches across multiple packages in the repo
  • You don't want to release any package until it is working end to end
  • You want to put a proper constraint on the packages in the same repo, to only allow the (unpublished) version, instead of relying on remembering to bump it later on when you are ready to publish.

I don't see a good workflow for this today. I don't have a solution to pitch yet either, but it might be worth thinking about.

@natebosch
Copy link
Member

cc @devoncarew - WDYT about adding a check in firehose for a package which has a more than +1 increment in any semver position over what is published? I can imagine edge cases with retracted packages. I'm not sure if there are other reasons to have a version bump that skips a number.

@jakemac53
Copy link
Contributor Author

Oh, I see Nate beat me too it :).

@jakemac53
Copy link
Contributor Author

Going to hold off just a bit on landing this, we have some changes that would be good to get out prior to dropping new feature support for anything <=3.4.

Nate also brought up the idea via chat of just pinning the -wip version exactly, which totally works and also requires us to update the constraint when we actually do a release, which is good. Pretty happy with that solution.

@devoncarew
Copy link
Member

cc @devoncarew - WDYT about adding a check in firehose for a package which has a more than +1 increment in any semver position over what is published? I can imagine edge cases with retracted packages. I'm not sure if there are other reasons to have a version bump that skips a number.

I think adding additional safety checks would be valuable. If they are checks that are mostly always things we want to enforce - but that have infrequent, legitimate, violations, we can either handle that via:

  • the occasional manual publish (not ideal but feasible)
  • having a category of checks that can be disabled via adding a label on the PR; allow people to opt-out if they think it doesn't allow for a specific change

@jakemac53 jakemac53 merged commit 14f9b3e into master Jul 8, 2024
39 checks passed
@jakemac53 jakemac53 deleted the use-worksapces branch July 8, 2024 16:30
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jul 8, 2024
…em, file, glob, http, http_multi_server, http_parser, json_rpc_2, logging, mockito, package_config, source_maps, source_span, sync_http, test, yaml_edit

Revisions updated by `dart tools/rev_sdk_deps.dart`.

args (https://github.com/dart-lang/args/compare/6a5a2e6..1a24d61):
  1a24d61  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/args#278)

bazel_worker (https://github.com/dart-lang/bazel_worker/compare/c76d7c8..02f190b):
  02f190b  2024-07-01  Kevin Moore  blast_repo fixes (dart-lang/bazel_worker#94)

benchmark_harness (https://github.com/dart-lang/benchmark_harness/compare/f6ef33d..a06785c):
  a06785c  2024-07-01  Kevin Moore  blast_repo fixes (dart-lang/benchmark_harness#108)

collection (https://github.com/dart-lang/collection/compare/9354f38..0c1f829):
  0c1f829  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/collection#354)

ecosystem (https://github.com/dart-lang/ecosystem/compare/54ca01a..4171189):
  4171189  2024-07-02  Jacob MacDonald  support nested packages in firehose (dart-lang/ecosystem#277)
  459041b  2024-07-01  dependabot[bot]  Bump the github-actions group with 4 updates (dart-lang/ecosystem#275)

file (https://github.com/google/file.dart/compare/07cacae..855831c):
  855831c  2024-07-01  dependabot[bot]  Bump actions/checkout from 4.1.6 to 4.1.7 (google/file.dart#242)
  da79121  2024-07-01  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.4 to 1.6.5 (google/file.dart#241)

glob (https://github.com/dart-lang/glob/compare/6d3ba5e..8b05be8):
  8b05be8  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/glob#96)

http (https://github.com/dart-lang/http/compare/8d89385..4178b67):
  4178b67  2024-07-03  Brian Quinlan  Clarify when Client.close must be called (dart-lang/http#1255)
  719dc5f  2024-07-02  Brian Quinlan  Upgrade to http_image_provider: 0.0.3 (dart-lang/http#1253)
  75b1efb  2024-07-02  dependabot[bot]  Bump the github-actions group across 1 directory with 4 updates (dart-lang/http#1251)
  cdfb94c  2024-07-01  Brian Quinlan  Add an section explaining the benefits of using `package:ok_http`. (dart-lang/http#1252)

http_multi_server (https://github.com/dart-lang/http_multi_server/compare/25941e2..8348be1):
  8348be1  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/http_multi_server#71)

http_parser (https://github.com/dart-lang/http_parser/compare/9bf7bd9..ce528cf):
  ce528cf  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/http_parser#101)

json_rpc_2 (https://github.com/dart-lang/json_rpc_2/compare/616937f..b4810dc):
  b4810dc  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/json_rpc_2#117)

logging (https://github.com/dart-lang/logging/compare/6c3fb37..8752902):
  8752902  2024-07-01  Kevin Moore  blast_repo fixes (dart-lang/logging#170)

mockito (https://github.com/dart-lang/mockito/compare/a7fdf71..eb4d1da):
  eb4d1da  2024-07-03  James Lin  Update with review feedback from srawlins
  db19e8c  2024-06-05  James Lin  Provide better documentation for `provideDummy`/`provideDummyBuilder`
  330976e  2024-07-01  dependabot[bot]  Bump the github-actions group across 1 directory with 2 updates (dart-lang/mockito#761)

package_config (https://github.com/dart-lang/package_config/compare/903a0e5..f0b7256):
  f0b7256  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/package_config#156)

source_maps (https://github.com/dart-lang/source_maps/compare/caa79c2..5f82c61):
  5f82c61  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/source_maps#95)

source_span (https://github.com/dart-lang/source_span/compare/89520f3..f81cd4a):
  f81cd4a  2024-07-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/source_span#115)

sync_http (https://github.com/dart-lang/sync_http/compare/7622bdd..ab8377e):
  ab8377e  2024-07-01  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.2 to 1.6.5 (google/sync_http.dart#48)

test (https://github.com/dart-lang/test/compare/3256c23..14f9b3e):
  14f9b3ec  2024-07-08  Jacob MacDonald  use pub workspaces (dart-lang/test#2249)
  c14ce93a  2024-07-03  Nate Bosch  Prepare to publish (dart-lang/test#2250)

yaml_edit (https://github.com/dart-lang/yaml_edit/compare/57a28da..d605cce):
  d605cce  2024-07-04  Kavisi  Fix fold literal encoding with trailing line break (dart-lang/yaml_edit#91)

Change-Id: I70ee32b3fa1912457b7b08affb3446523ef3b0d1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/374860
Reviewed-by: Brian Quinlan <bquinlan@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
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.

4 participants