-
Notifications
You must be signed in to change notification settings - Fork 1
feat(dart): create Dart feature #23
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
Conversation
I know https://github.com/jarrodcolburn/features/tree/main/src/flutter-sdk has the Flutter SDK feature listed on https://containers.dev/features but it seems like @jarrodcolburn has archived it. Maybe that would be another good candidate for a feature @eitsupi? We need to formally define guardrails in #17 though. |
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 think Dart is worth adding here as it is required for devcontainers-community/templates#31.
Added a few comments that may help with the fix. Also, please add tests.
src/dart/install.sh
Outdated
# "Perform the following one-time setup:" | ||
sudo apt-get update | ||
sudo apt-get install apt-transport-https | ||
wget -qO- https://dl-ssl.google.com/linux/linux_signing_key.pub | sudo gpg --dearmor -o /usr/share/keyrings/dart.gpg |
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.
Should check wget
installed before this 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.
Does check_packages wget
do this, or do I also need the ca-certificates
or whatever magic?
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.
How about rewrite it to the same process as the official Feature?
https://github.com/devcontainers/features/blob/852f1f0567280b465913b487d5f8c773122c36ab/src/node/install.sh#L99-L103
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 tried to stay as close to the official https://dart.dev/get-dart way as I reasonably could (hence my earlier mistake with sudo 🤦♂️).
I note specifically this part:
# Yarn example
echo "deb [arch=$(dpkg --print-architecture) ..."
# The official https://dart.dev/get-dart
echo 'deb ... arch=amd64]'
Is this significant? Are there other significant differences between https://dart.dev/get-dart and the way it's done in the Node.js yarn thing in the devcontainers/features repo?
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 this significant? Are there other significant differences between dart.dev/get-dart and the way it's done in the Node.js yarn thing in the devcontainers/features repo?
This difference is very important and must be emphasized in the documentation because if Dart only supports amd64, the arm64 platform will not be able to use this Feature.
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.
Unfortunately, according to the official site, installation via apt is only supported on amd64, so we need to switch to downloading the zip file and installing that.
Could you please change the installation method completely?
Would it be acceptable to add a big notice and merge this as a minimum working thing? Then do a subsequent PR? |
I believe this PR should be completely rewritten before merging. Since I have a little time, I will take care of this task. |
Continued from #23 --------- Co-authored-by: Jacob Hummer <[email protected]>
There is no "Dart" feature on https://containers.dev/features yet. I would like to add this feature so that the devcontainers-community/templates#31 Dart template can use this Dart feature without needing to install it in a Dockerfile.
This PR would...
_common.sh
until [proposal] Sharing code between Dev Container Features devcontainers/spec#209 is implemented in tooling?version
option to choose the version of the Dart SDKI am not familiar with the Dart ecosystem enough to know whether there are any other tools or things that should be installed or options that should be given. For instance, should https://flutter.dev/ be a separate feature that depends on Dart or rolled into the same one? Probably separate, but I'm not in-tune enough to know 100%.
Nevertheless I think that merging this basic Dart feature would be good to get devcontainers-community/templates#31 flying even if #17 is not finished yet.