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

Upload releases for musl-libc and android #2149

Merged
merged 18 commits into from
Dec 15, 2023
Merged

Upload releases for musl-libc and android #2149

merged 18 commits into from
Dec 15, 2023

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Dec 14, 2023

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

@ntkme
Copy link
Contributor Author

ntkme commented Dec 14, 2023

Sample run (some release steps are mocked): https://github.com/ntkme/dart-sass-test/actions/runs/7216265257
Sample release: https://github.com/ntkme/dart-sass-test/releases/tag/1.69.6


Screenshot 2023-12-14 at 3 04 27 PM
Screenshot 2023-12-13 at 9 13 33 PM

@farnetto
Copy link

The musl version works well for us. Thanks!

@ntkme ntkme marked this pull request as draft December 14, 2023 19:36
@ntkme ntkme marked this pull request as ready for review December 14, 2023 21:09
Copy link
Contributor

@nex3 nex3 left a 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!

Copy link
Contributor

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.

Copy link
Contributor Author

@ntkme ntkme Dec 15, 2023

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:

Copy link
Contributor Author

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...

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)

.github/workflows/build-linux-musl.yml Show resolved Hide resolved
.github/workflows/build-linux-musl.yml Outdated Show resolved Hide resolved
.github/workflows/build-linux-musl.yml Outdated Show resolved Hide resolved
.github/workflows/build-linux-musl.yml Outdated Show resolved Hide resolved
Comment on lines 63 to 65
# 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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@ntkme ntkme Dec 15, 2023

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.

.github/workflows/build-windows.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
ntkme and others added 2 commits December 14, 2023 17:52
Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
@ntkme ntkme requested a review from nex3 December 15, 2023 02:24
@stof
Copy link
Contributor

stof commented Dec 15, 2023

should we also add support for musl-libc in the embedded-host-node repository so that the npm package sass-embedded can have this executable among its dependencies as well ?

@ntkme
Copy link
Contributor Author

ntkme commented Dec 15, 2023

@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.

@nex3 nex3 merged commit 1fc740d into sass:main Dec 15, 2023
34 checks passed
@ntkme ntkme deleted the release-linux-extra branch December 15, 2023 22:34
@nex3
Copy link
Contributor

nex3 commented Dec 28, 2023

@ntkme Looks like the release is failing to find /.../dart/bin/snapshots/gen_kernel_aot.dart.snapshot for linux-musl 386 and android amd64. There's also a failure in the website deploy, which seems like npm isn't making the new package version available quickly enough. Not sure what we can do there other than add some additional wait time.

@ntkme
Copy link
Contributor Author

ntkme commented Dec 28, 2023

@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...

@ntkme
Copy link
Contributor Author

ntkme commented Dec 28, 2023

@nex3 I think the issue was this change: google/dart_cli_pkg@3c5a2cc

@nex3
Copy link
Contributor

nex3 commented Dec 28, 2023

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.

@ntkme
Copy link
Contributor Author

ntkme commented Dec 28, 2023

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.

jgerigmeyer added a commit to oddbird/dart-sass that referenced this pull request Jan 8, 2024
* 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)
lmeysel pushed a commit to lmeysel/dart-sass that referenced this pull request Feb 20, 2024
Co-authored-by: Natalie Weizenbaum <nweiz@google.com>
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.

Doesn't run on alpine linux
5 participants