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

Support renaming the be channel to main. #102

Merged
merged 4 commits into from Sep 13, 2023

Conversation

sortie
Copy link
Member

@sortie sortie commented Sep 8, 2023

This change automatically detects when the main channel pops into existence and starts using the latest version of it instead. The forward compatibility will be removed once the channel has been renamed.

Bug: b/270022609

--

I couldn't figure out how to test this change. Could you add some instructions to DEVELOPING about how I can run and test my changes locally? Maybe I could catch a more specific exception or http status code to perform the fallback more cleanly so a transient failure doesn't downgrade from main to be.

No particular timetable on when we'll rename the be channel to main. I actually believe this is the final piece of forward compatibility needed but I'll need to confirm that.

@sortie
Copy link
Member Author

sortie commented Sep 8, 2023

The tests appear to fail like my attempts to run it locally.

I literally just ran:

sudo apt-get install nodejs
sudo npm i -g @vercel
dart pub get # on 3.2.0-134.0.dev
npm run all

Please advise and update DEVELOPING if I'm doing something wrong?

@devoncarew
Copy link
Member

I'll review and see if I can spot where this is having problems. In the meantime, is there a github issue you can reference (from the PR and the code) instead of a buganizer issue? Even if the two issues are cross-referenced, it would help w/ visibility.

@devoncarew
Copy link
Member

sudo apt-get install nodejs
sudo npm i -g @vercel
dart pub get # on 3.2.0-134.0.dev
npm run all

Please advise and update DEVELOPING if I'm doing something wrong?

Those steps look correct (the setup and re-compiling the javascript code), and matches whats in https://github.com/dart-lang/setup-dart/blob/main/DEVELOPING.md. From the failures it does look like there's some issue with the javascript produced. Perhaps due to differing versions of node or of dart2js?

lib/main.dart Outdated
// automatically migrates users when the be channel is renamed to main.
try {
channel = 'main';
version =
Copy link
Member

Choose a reason for hiding this comment

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

Note that the latestPublishedVersion method won't be called for all paths here, like if the raw flavor had been specified. I take it we're trying to validate that a url exists for the main channel, and if no, fallback on the be channel? If so we'd need to validate the urls for the raw flavor as well.

Will this be a very quick transition? Using be is I think a pretty specialized use case. We could do something like start double-publishing to main and be now, have the (current) 1.5.0 version of this action read from be, and publish a newer 1.6.0 version which only read from main.

And I know there are some used cases for having a raw flavor, but they're so rarified I wonder if we couldn't remove that as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I missed the raw variable.

Only the raw flavor makes sense for main/be and only the latest version, since we don't publish or sign the main channel. I updated the temporary logic to handle the supported cases.

I tend to support removing the flavor support as an explicit input and instead default to the raw flavor on main only. I'll clean this up in a follow up change when we've done the migration.

I hope to do the migration this week or the next one, no big rush. Deploying this package may be a limiting factor on how fast we can roll it out? But the main channel isn't really that supported.

The migration will happen as an atomic switch when tools/VERSION in the Dart SDK is modified to the new channel. The rest of the infrastructure will respect the channel name in there and start uploading to the new places. Services like api.dart.dev and setup-dart will handle the switch automatically when the new channel pops into existence. The infra isn't quite set up to upload to both places though we could potentially do that, it just isn't the migration path I've planned out at this time.

Copy link
Member

Choose a reason for hiding this comment

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

Deploying this package may be a limiting factor on how fast we can roll it out?

We can release this at any time - it's just creating a github release (https://github.com/dart-lang/setup-dart/wiki/Publishing-procedure). I can take care of that after your work lands.

@devoncarew
Copy link
Member

The bulk of this action is tested via the various matrixs in this file: https://github.com/dart-lang/setup-dart/blob/main/.github/workflows/dart.yml

I did make a small change to a file and verified that my version of node and dart sdk produces working javascript: #103.

Dart SDK version: 3.2.0-134.0.dev
ncc: 0.36.1

@sortie sortie force-pushed the sortie-support-renaming-be-to-the-main-channel branch from bb0680b to bb97d87 Compare September 12, 2023 12:37
This change automatically detects when the main channel pops into
existence and starts using the latest version of it instead. The forward
compatibility will be removed once the channel has been renamed.

Bug: b/270022609
@sortie sortie force-pushed the sortie-support-renaming-be-to-the-main-channel branch from bb97d87 to 1d5b453 Compare September 12, 2023 12:46
@sortie
Copy link
Member Author

sortie commented Sep 12, 2023

I addressed the code review but unfortunately the tests are still broken when I use the same versions as you.

sortie@sortie ~/src/setup-dart $ npm --version
9.2.0
sortie@sortie ~/src/setup-dart $ sudo npm i -g @vercel/ncc@0.36.1

changed 1 package in 2s
sortie@sortie ~/src/setup-dart $ ncc --version
0.36.1
sortie@sortie ~/src/setup-dart $ dart --version
Dart SDK version: 3.2.0-134.0.dev (dev) (Mon Sep 4 13:06:39 2023 -0700) on "linux_x64"
sortie@sortie ~/src/setup-dart $ dart pub get
Resolving dependencies... 
Got dependencies!
Resolving dependencies in ./example... (1.4s)
Got dependencies in ./example.
sortie@sortie ~/src/setup-dart $ npm run all

> setup-dart@0.0.0 all
> npm run build && npm run dist


> setup-dart@0.0.0 build
> dart compile js --enable-experiment=inline-class -olib/main.js lib/main.dart && dart tool/sig.dart --generate

Hint: When run on the command-line, the compiled output might require a preamble file located in:
  <sdk>/lib/_internal/js_runtime/lib/preambles.
Compiled 9,932,454 input bytes (5,130,131 characters source) to 357,512 characters JavaScript in 1.92 seconds using 236.555 MB of memory

> setup-dart@0.0.0 dist
> ncc build lib/main.mjs && cp lib/main.js dist/main.cjs && cp lib/sig.txt dist/sig.txt

ncc: Version 0.36.1
ncc: Compiling file index.mjs into ESM
12kB  dist/index.mjs
12kB  [662ms] - ncc 0.36.1

Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

The changes look good; can you update the buganizer ref, and, add appropriate notes to the changelog? We'd want to mention the auto-handling of the old and new channels, and the changed behavior w/ versions and the main channel.

Also - as a point of workflow for github PRs - you don't want to force push to a branch that's out for review; just continue to add commits to it, and then squash them all to the default branch after the review's complete. If you force push than the comments from a review get disassociated with commits from the PR.

Also - it looks like I am able to build artifacts that work; not sure what the difference is. After your updates I'll rebuild the action, then merge this PR.

lib/main.dart Outdated
channel = 'be';
version =
raw ? 'latest' : (await latestPublishedVersion(channel, flavor));
// TODO(b/299435467): Temporary forward compatibility that
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace this buganizer reference with a github issue ref?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could do that though the issue is just for myself to track the cleanups -- our team uses buganizer internally rather than github issues to manage our daily tasks and I never look at my github issues in practice. There's literally no content in the bug besides the title which is repeated here -- it just serves as an unique identifier that I can code search for to find all the relevant code locations that needs fixing. Do you feel I should file an issue in this repo about this cleanup instead?

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. My main concern was that users w/o access to the bug would have some mystery about what it was tracking. I re-worded this comment slightly to clarify things (the buganizer issue just tracks the channel rename).

@sortie
Copy link
Member Author

sortie commented Sep 13, 2023

Ah good point about force pushes. A habit from my work in other code hosting solutions hehe. Thanks for recompiling for actions for me. Although I really would love if we could figure out why I can't compile them myself, or if there's something special about your setup. I added a CHANGELOG entry :)

@devoncarew
Copy link
Member

Although I really would love if we could figure out why I can't compile them myself, or if there's something special about your setup.

Yeah, I definitely agree here - I want to make sure everyone can contribute; probably worth tracking in an issue to see if other people hit it / have ideas.

Thanks for updating the action!

@devoncarew devoncarew merged commit 8a4b97e into main Sep 13, 2023
24 checks passed
@devoncarew devoncarew deleted the sortie-support-renaming-be-to-the-main-channel branch September 13, 2023 16:50
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.

None yet

2 participants