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

build_web_compilers shouldn't have an upper SDK bound. #3598

Open
sortie opened this issue Oct 5, 2023 · 17 comments
Open

build_web_compilers shouldn't have an upper SDK bound. #3598

sortie opened this issue Oct 5, 2023 · 17 comments
Labels

Comments

@sortie
Copy link

sortie commented Oct 5, 2023

Somebody needs to fix build_web_compilers so it doesn't have an upper sdk limit because it blocks our ability to make releases. Right now if we bump the sdk version, the test will fail, blocking out ability to update the sdk version after cutting a stable release.

The tests

pkg/dartdev/test/commands/create_integration_test RuntimeError (expected Pass)

are failing on configurations

unittest-asserts-release-linux

https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8768040075748241681/+/u/test_results/new_test_failures__logs_

@lrhn lrhn changed the title build_web_compilers doesn build_web_compilers shouldn't have an upper SDK bound. Oct 6, 2023
@lrhn lrhn added the area-web label Oct 6, 2023
@jakemac53
Copy link
Contributor

The upper bound isn't just there for no reason... we have a somewhat tight upper bound as an agreement with the web compiler team, because it allows those tools (and others) to make breaking changes to their command line arguments, etc.

If we are notified in advance of the version change, we could pre-emptively bump the max SDK limit ahead of time?

@jakemac53
Copy link
Contributor

New versions have been published that allow 3.3.x, not closing this for now though since I think there is more discussion to be had for future releases.

@jakemac53
Copy link
Contributor

Fwiw - we have a similar situation with dart_internal. So whatever process we have for that package could be also used for these packages? For macros we also plan to have another package that will be tightly constrained to SDK versions.

@sortie
Copy link
Author

sortie commented Oct 6, 2023

Thanks for the quick answers. Yeah we'll definitely need to have solutions here and I'm happy to help find them :)

Because right now it can be pretty impossible to start work on the next dart version and we need to significantly reduce issues like this

@jakemac53
Copy link
Contributor

I am definitely happy to do a pre-emptive bump. It typically should not take more than a day of turnaround time. I just don't know how to coordinate that with the releases.

These pre-emptive version bumps could go out even a couple weeks before the actual release.

@sortie
Copy link
Author

sortie commented Oct 6, 2023

go/dash-team-releases has a lot of information and a spreadsheet where you can register yourself for action items that must be taken during a release. :)

Once the 3.3 bump goes through, I'll learn more about the classes of problems that people need solved and try to make this process easier for everyone :)

@jakemac53
Copy link
Contributor

jakemac53 commented Oct 6, 2023

I added a row to the release checklist template under the dart_internal bump

@jakemac53
Copy link
Contributor

Is that sufficient to close this issue? Presumably I will get assigned a task for future releases?

@sigmundch
Copy link
Member

mmm - something seems odd to me from the checklist process, I believe this needs to happen before we bump the version in the SDK, but currently we are thinking about this in terms of the release steps after the bump.

@fishythefish fishythefish transferred this issue from dart-lang/sdk Oct 17, 2023
@sortie
Copy link
Author

sortie commented Jan 4, 2024

This is happening again for the 3.4 version bump. Please publish a version with build_web_compilers that supports 3.4 :)

@itsjustkevin I think this release step somehow didn't get onto the release instructions, can you work with these build_web_compilers folks can get into the release procedures? This should ideally happen before we do the final merge to beta, where we do the main version bump immediately after.

Can you give us a deeper explanation of why this package needs an upper sdk constraint and why alternatives aren't possible? It's a bit of an antipattern whenever packages need to do this and this package is the only such package that causes problems for our release bumps -- a process that's already too complicated and risky. You mentioned something about needing to keep the package in sync with the Dart SDK and wanting to bypass the breaking change policy. Might it be possible to build a stable interface between this package and the dart sdk, so it can learn what it needs from the Dart SDK and support future releases? The breaking change policy is something of a feature in this regard rather than a problem since it encourages us to build stable interfaces that are durable. On the other hand, if this package is that tightly coupled with the Dart SDK, then perhaps it should be developed as part of the Dart SDK? That's where we got all the other packages that need the tight coupling. I'm curious to learn more so we can simplify and derisk our release procedures :)

@itsjustkevin
Copy link

Thanks for tagging me @sortie, this may be a good time for a full review and revamp of the release checklist, as many processes have evolved.

@natebosch
Copy link
Member

You mentioned something about needing to keep the package in sync with the Dart SDK and wanting to bypass the breaking change policy.

Yes, this is to give the web compiler team some flexibility for changes to the CLI. If we want to add more of the CLI to our breaking change policy I think it should be discussed with @sigmundch

On the other hand, if this package is that tightly coupled with the Dart SDK, then perhaps it should be developed as part of the Dart SDK?

We might be able to do that, potentially with this package or with a new package like _web_compiler_interface where we try to make a more stable Dart API on top of the potentially unstable CLI. I don't have an estimate on the complexity of splitting the package, or how well we would be able to keep that Dart API stable.

@jakemac53
Copy link
Contributor

Basically what Nate said, this package has a tight SDK dependency because it utilizes directly various tools shipped with the SDK. As an example, sometimes certain resources (SDK summaries for example), change their location or format. Sometimes snapshots are deleted entirely, and we move to utilizing dart compile commands. Sometimes command line arguments change. Sometimes features which are generally non-breaking are breaking for this package (such as null safety), because the tools have to be used differently.

Having the package be tightly versioned with the SDK helps to shield such breakage from our users, by ensuring they have a version compatible with their SDK.

I do not believe we can reasonably move this package into the SDK, because we do not support SDK package dependencies for Dart, and this package is built upon lots of other pub packages so it cannot reasonably be a dart: library. We could as Nate suggested try to create a new dart: library that abstracts away all the coupling that we currently have, but it would likely be pretty complex, and we would not be able to evolve it easily given that it would be a dart: library. We could ship that as a package too, but then we are in the same situation as today.

@sigmundch
Copy link
Member

Also, to add to Nate & Jake's comments, I believe tightening breaking change policies for CLI APIs to avoid this issue would likely be too strict. The one policy I can imagine would be to bump Dart's major version every time we change internals of the CLI like those Jake mentioned (flags, snapshots, sdk file locations, etc). That seems a bit too much, and probably more trouble than the current approach (since it will require bumping a lot more packages out there).

Usually tests on this package run on every dev release, so breakages are detected before they go out on the beta/stable channels. I believe that's sufficient to prepare ahead and adjust constraints early. By the time we are doing branch alignment for a minor version bump, we know already whether we need a new release with broader constraint or a new release with a non-overlapping constraint.

@sortie / @itsjustkevin - aside from the technical ideas like creating a new library/package, is there anything you'd like to do to make the release process go smoother? For example, I believe now we have included a step of publishing this package in the release checklist, right? Is it at the right time in the process? Would it help to do the publishing at a different time (e.g. before the branch is created and branch alignment starts?)

@jakemac53
Copy link
Contributor

+1 I think this is a process issue and not a technical issue. We can publish these versions well in advance, we just need to be notified to do so. Afaik we don't have similar issues with package:dart_internal, which does similar pinning. And should do the same for package:frontend_server_client really, but we accidentally published one of those with a wide constraint, so we have to wait until Dart 4.x to start that back up.

@natebosch
Copy link
Member

I do not believe we can reasonably move this package into the SDK

I think the suggestion was to continue publishing on pub, but to move development to the SDK repo, so the SDK version bump and the change to the dependency constraint can happen together. If we think build_web_compilers is more tightly coupled to the SDK than to the build packages, moving it could make sense, but I'm skeptical it would be worth it.

@jakemac53
Copy link
Contributor

but to move development to the SDK repo, so the SDK version bump and the change to the dependency constraint can happen together

Ah, you still have to publish to pub though? I believe the tests that fail are pulling build_web_compilers from pub, as they are testing the true user experience end to end. If we used an override in the tests, we risk forgetting to publish prior to the real release, and we lose general validation of dependency constraints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants