-
Notifications
You must be signed in to change notification settings - Fork 38
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
Install a Flutter SDK in the publish workflow #141
Conversation
# | ||
# This will add the flutter/bin/dart executable to path, that sets up | ||
# FLUTTER_ROOT. | ||
- uses: flutter-actions/setup-flutter@d030cb603380106494f72d65a7e52462f380781f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dart
binary from here will shadow the one installed from the previous step (dart-lang/setup-dart
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I don't know if this is an improvement - I haven't thought through the use cases - but we could parameterize this workflow (workflow_call
> inputs
, above) to indicate which sdk should be set up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any downside to using the Dart SDK from Flutter to publish?
It's a Dart SDK like the other one, the only difference is that it can resolve flutter dependencies.
It's not exactly a pretty solution, but we can always add more inputs in the future. And if we can do more without asking people to make a special case that would be nice :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of any downsides (just that the optics are a little strange esp. given that this workflow is hosted in the dart-lang/setup-dart
repo).
No concerns w/ this however. I would remove the line above - uses: dart-lang/setup-dart
as that'll become a no-op given the uses: flutter-actions/setup-flutter
line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup-dart step is still important because it configures the OIDC token.
Gotcha. Perhaps update the comments in this file? 'Set up the Dart SDK and provision the OIDC token used for publishing'
; 'Set up the Flutter SDK; use the dart command from here to facilitate publishing both dart and flutter packages'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
The setup-dart step is still important because it configures the OIDC token.
ons. 20. nov. 2024, 20.27 skrev Devon Carew ***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In .github/workflows/publish.yml
<#141 (comment)>:
> @@ -31,6 +31,12 @@ jobs:
- uses: ***@***.***
# Setup Dart SDK with JWT token
- uses: ***@***.***
+ # Download flutter SDK - needed for publishing Flutter packages. Can also
+ # publish pure Dart packages.
+ #
+ # This will add the flutter/bin/dart executable to path, that sets up
+ # FLUTTER_ROOT.
+ - uses: ***@***.***
I can't think of any downsides (just that the optics are a little strange
esp. given that this workflow is hosted in the dart-lang/setup-dart repo).
No concerns w/ this however. I would remove the line above - uses:
dart-lang/setup-dart as that'll become a no-op given the uses:
flutter-actions/setup-flutter line.
—
Reply to this email directly, view it on GitHub
<#141 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABERZCIPI6U2R7KD6MZJ2T2BTPBNAVCNFSM6AAAAABSB73PBGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBZGUYTOMJRGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Supposed to fix #68
Validated that the pub-get is able to resolve a flutter package in: https://github.com/sigurdm/private_repo2/actions/runs/11913212325/job/33198427010#step:6:109
Ideally we would have a flutter provided setup action, not a community provided one. But given that we can lock to a specific revision of the action I think this is still safe.
Ideally we only install the flutter sdk when needed, but with the frequency of publishing that is a very minor problem.