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 sdk version wildcard format #106

Merged
merged 8 commits into from Sep 29, 2023
Merged

Conversation

devoncarew
Copy link
Member


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@devoncarew
Copy link
Member Author

@sortie - do you mind reviewing this?

runs-on: ubuntu-latest
strategy:
matrix:
sdk: [2.19.x, 3.1.x]
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered using ^2.19.0 instead? Then this would be more similar to how you specify SDK constraints, maybe we could even support more complicated SDK constraints in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I haven't. We had a feature request for 2.19.x, and I believe this may match the action support for one other language.

We could also consider just 2.19; using just two version segments could be an implicit way to opt-into using the latest patch release for a version.

^2.19.0 could also work. The current way this PR is implemented, we couldn't efficiently have an actual semver match - like getting the latest version that matches a version range (i.e., '>=2.18.3 <2.19.2', ...). We could do that if we have a json method we could call that either returned all the available sdk versions, or, all the versions that matched a given range. We're currently relying on a very specific usage of the google storage list API. It works the way we want it for this specific use case, but for anything more sophisticated we'd need to have all the sdk versions available (perhaps read from a single well-known file in google storage - https://storage.googleapis.com/dart-archive/channels/stable/release/versions.json?).

Copy link
Member

Choose a reason for hiding this comment

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

Up to you :)

Either option will work. Caret syntax feels more "Dart". Not sure if we ever need more complicated constraints (definitely wouldn't implement it now without a use case).

Copy link
Member

Choose a reason for hiding this comment

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

We could also just have e.g. "2.19" to select the latest 2.19.x release

Agreed that caret syntax feels dart-y although it also does suggest the powerful of the whole pub version resolution

Copy link
Member

Choose a reason for hiding this comment

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

The caret is actually kinda bad since ^3.1.0 means up to 4.0, which isn't the semantics we try to do here.

Copy link
Member

Choose a reason for hiding this comment

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

That's true, caret means until the next major, so it doesn't quite cover the use case of latest patch version. So for that we'd need to support pub-like version ranges which seems inconvenient. So I think rather than making this too complicated we should look at other languages and make a "GitHub-y" solution.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like most setup- actions support: 3, 3.2, 3.4.5. The .x is probably supported as well mostly because https://github.com/npm/node-semver supports it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sortie - good point about the caret syntax above.

I'd be fine w/ either 2.19 or 2.19.x. They're both simple. If the two version variant (2.19) is more common for github actions then we should just do that. We could support both patterns but I don't think the flexibility is really necessary here.

I'll re-work this PR to use 2.19.

@devoncarew
Copy link
Member Author

I updated the PR to use 2.19 (and, rev'd the version of the action to 1.6.0 - this PR essentially adds new API to the action).

CHANGELOG.md Outdated Show resolved Hide resolved
- run: dart --version

# Test using wildcard versions for the sdk parameter.
test_release_wildcards:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: test_latest_patch_release (and # Test getting the latest patch release for a major.minor sdk parameter.)

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

(btw, you can make in-line suggested edits to PRs using the 'add a suggestion' action)

final versionPrefix = sdk.substring(0, sdk.length - '.x'.length);
version = await findLatestSdkForRelease(versionPrefix);
// Find the latest version for the given sdk release.
version = await findLatestSdkForRelease(sdk);
Copy link
Member

Choose a reason for hiding this comment

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

How about the same but for beta releases? E.g. the latest version in a beta series? Or maybe 3.3.0-1.beta would give you 3.3.0-1.1.beta and later 3.3.0-1.2.beta and onwards.

Although in those cases you can kinda just follow the beta channel, although that would give potential breakages whenever a new beta series is dropped, whereas a patched beta is only more stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean something like 3.1-beta? I.e., the latest beta for a given sdk release?

I don't think that's a common use case. I think testing against the latest stable is common, and testing against the upcoming releases (beta, dev, ...) is common. What this PR will help with is for people that want to test against specific SDKs (for example, the lower sdk constraint of their pubspec), but want the latest patch release for that SDK. Right now they're using 3.1.0 as a stand-in for that lower bounds, but I think they really want to test against 3.1.<latest patch>.

@devoncarew
Copy link
Member Author

Thanks for the feedback and discussion! Much appreciated.

@devoncarew devoncarew merged commit 8ef13bc into main Sep 29, 2023
26 checks passed
@devoncarew devoncarew deleted the support_version_wildcards branch September 29, 2023 03:54
@devoncarew
Copy link
Member Author

#107

@parlough
Copy link
Member

\cc @kevmoo I really like this since it's can resolve the main issue I had with the pubspec SDK version specifier for package:mono_repo. If my lowerbound was ^3.0.0, the CI wouldn't run with later patch fixes until I changed the lower bound and regenerated.

Do you think a new pubspec-minor (or some similar name) choice would make sense after this is stabilized, that would use 3.0 instead of 3.0.0?

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.

Support version segment placeholder in the sdk constraint
4 participants