-
Notifications
You must be signed in to change notification settings - Fork 359
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
Upload releases for musl-libc and android #2149
Conversation
Sample run (some release steps are mocked): https://github.com/ntkme/dart-sass-test/actions/runs/7216265257 |
The musl version works well for us. Thanks! |
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.
This refactor looks really nice, thanks for taking the time!
.github/workflows/build-android.yml
Outdated
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.
Have we ever had anyone request native Android support for Dart Sass? I'm not sure this is worth the trouble.
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.
There were a few people asking this from jekyll community. Interestingly, now there are some (very few) active users:
- https://rubygems.org/gems/sass-embedded/versions/1.69.5-aarch64-linux-android (downloaded 242 times as of writing)
- https://rubygems.org/gems/sass-embedded/versions/1.69.5-arm-linux-androideabi (downloaded 159 times as of writing)
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 guess another point is SWC (https://swc.rs/) also has native android support: https://www.npmjs.com/package/@swc/core-android-arm64
This means there are people out there using android for web development...
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.
Have we ever had anyone request native Android support for Dart Sass? I'm not sure this is worth the trouble.
Now that dart-sass has an Android variant, I actually had a request to get my embedded host working on Android:
larsgrefer/dart-sass-java#461 (comment)
# The kernel snapshot created for arm in the previous step is bundling a glibc based dart runtime | ||
# due to how cli_pkg download the sdk for building non-native platforms. Therefore we need to | ||
# replace it with musl-libc based dart runtime to create a working linux-musl-arm package. |
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.
Would it make sense to build more of this logic into cli-pkg
? We could teach it pkg-standalone-linux-musl-*
with your SDK releases.
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.
That's certainly possible, but I would consider that at a later time due to the complexity. The problem mainly lies on dart-sdk only have the knowledge of itself running as some "linux" variant, and have no concept of "linux-gnu" or "linux-musl", which would require lots of structural changes to cli_pkg to accommodate this. Also, we need a way to know what is the current running variant, that either the current running dart-sdk need to be able to tell what kind of linux variant by itself, or we need to parse the dart executable file as ELF, and then detect what it is based on the interpreter field.
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.
Would it be possible for your fork to add "musl" somewhere in the dart --version
output? That seems like the easiest way to tell—we don't need an actual API call in the SDK itself necessarily.
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.
My "fork" does not modify any dart-sdk source code. Only thing it does is running a CI/CD with a different build toolchain for building dart-sdk on alpine linux.
The reason I said it is difficult to do in either dart or cli_pkg is because the platform logic in dart is closely tied to the Abi
class in ffi
package. For example, the dart --version
pretty much prints Abi.current().toString()
. In order to print musl, we can possibly either add new constant, but this will likely break existing code that does the platform detection via Abi
class; or modify dart --version
to hard code a different string instead of using constant, which again might be unexpected.
I don't think change the version string is the correct behavior. I think the right way to deal with this in dart-sdk would be to provide new API that returns the "target triplet". For example, in ruby we have RbConfig::CONFIG['arch']
which would return x86_64-linux-musl
that clearly identify the right platform.
Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
should we also add support for musl-libc in the embedded-host-node repository so that the npm package |
@stof I’m planning to look into that after this change goes out. We don’t necessarily need to support both at the same time. |
@ntkme Looks like the release is failing to find |
@nex3 It looks like there might be a change in dart SDK causing cli_pkg to regress... for any ia32 platform it should not attempt to generate aot-snapshot... |
@nex3 I think the issue was this change: google/dart_cli_pkg@3c5a2cc |
Oh, I see... I skimmed dart-lang/sdk#47177 and just noticed the fact that it was closed as completed and my comment that I was testing against the wrong SDK and assumed it was fixed. I'll re-add the exemption. |
The previous comment in the cli_pkg was indeed a bit misleading about all 32-bit. Dart SDK do have arm 32-bit aot support, but it never had ia32 aot support (see comments in dart-lang/sdk#49969). In fact, the implementation was correct that it only exempt ia32. |
* main: Update the pubspec and changelog for sass/embedded-host-node#266 (sass#2158) Add wait time before update website (sass#2153) Make meta.apply() an AsyncBuiltInCallable (sass#2152) Upload releases for musl-libc and android (sass#2149) Escape unprintable 0x7F (delete control character) (sass#2144) Bump dartdoc from 7.0.2 to 8.0.2 (sass#2146)
Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
This PR creates release assets for linux-musl and android using unofficial dart-sdk builds that I'm maintaining:
This PR also does a refactor of CI job compositions to make it easier to understand and maintain. A behavior difference is that now the GitHub Release is only created if all the builds are successful, which avoid the issue we had in the past that some of the release artifact were missing due to random issues. In other words, the release assets are either all or none.
Closes #2148