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 musl-libc and android #265

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Support musl-libc and android #265

merged 3 commits into from
Dec 18, 2023

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Dec 16, 2023

This PR adds support for musl-libc and android (and windows-arm64).

Because npm optionalDependenices only knows about generic linux, we have to install two variants on all linux system, and pick the correct one to run at runtime. It's not a huge problem other than waste some network bandwidth for download and a small amount of disk space.

Closes #263

@ntkme
Copy link
Contributor Author

ntkme commented Dec 16, 2023

The biggest problem here is we must install both -gnu and -musl at the same time, as that is a limitation of optional dependencies. For the embedded host it is not an issue, as we can detect the runtime and pick one to use, however it is a problem for the cli, as now we have two packages trying to install a bin with the same name.

  "bin": {
    "sass": "./dart-sass/sass"
  },

One way to solve this is to use a wrapper script to run the same platform detection logic and then invoke the dart sass command. The problem here is that node does not have proper support for exec call that replaces the current process, so that the node js wrapper need to stay alive until the child process exit, which is a minor performance issue.

@ntkme ntkme marked this pull request as ready for review December 16, 2023 00:58
@nex3
Copy link
Contributor

nex3 commented Dec 16, 2023

Do we need Node to have an exec? Since these packages are only ever going to be run on Linux anyway, could we have them ship a shell script to choose between them instead?

Also, does it make sense to have separate packages for glibc and musl, since they're both going to be installed anyway?

@ntkme
Copy link
Contributor Author

ntkme commented Dec 16, 2023

Do we need Node to have an exec? Since these packages are only ever going to be run on Linux anyway, could we have them ship a shell script to choose between them instead?

It is unfortunate that the issue is closed without progress: nodejs/node#21664

Also, does it make sense to have separate packages for glibc and musl, since they're both going to be installed anyway?

We don't really need two packages, however the logic is simpler in this way because all the optional packages have the exact same structure.

@nex3
Copy link
Contributor

nex3 commented Dec 16, 2023

It is unfortunate that the issue is closed without progress: nodejs/node#21664

I mean, what if we didn't make the frontend script Node at all, and just made it a shell script instead?

@ntkme
Copy link
Contributor Author

ntkme commented Dec 16, 2023

I mean, what if we didn't make the frontend script Node at all, and just made it a shell script instead?

I that case I'm not sure how to support posix shell and windows shell at the same time, unless we to with combined gnu/musl package, and have the shell script doing the detection work, in which way we can still go with the existing approach that each optional package contains one shell script.

@ntkme
Copy link
Contributor Author

ntkme commented Dec 16, 2023

Or I we can still use separate packages, but include a separate bash script for the linux (-gnu) package, and use that for command line. For -musl, there will be no command script. I think this might be the best compromise.

@nex3
Copy link
Contributor

nex3 commented Dec 16, 2023

I that case I'm not sure how to support posix shell and windows shell at the same time, unless we to with combined gnu/musl package, and have the shell script doing the detection work, in which way we can still go with the existing approach that each optional package contains one shell script.

If it's enough easier to have separate packages for gnu/musl, you could also just say "the gnu package is the only one with a script and that script determines which actual executable to load".

@ntkme
Copy link
Contributor Author

ntkme commented Dec 16, 2023

If it's enough easier to have separate packages for gnu/musl, you could also just say "the gnu package is the only one with a script and that script determines which actual executable to load".

I also had the same idea and we almost posted at exact same time :-)

@nex3 nex3 self-requested a review December 18, 2023 21:54
@@ -6,14 +6,34 @@ import * as fs from 'fs';
import * as p from 'path';
import {isErrnoException} from './utils';

const isLinuxMusl = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: document this (in particular why it works consistently)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment

@ntkme ntkme requested a review from nex3 December 18, 2023 22:48
@nex3 nex3 merged commit b52c9b0 into sass:main Dec 18, 2023
15 checks passed
@nex3
Copy link
Contributor

nex3 commented Dec 18, 2023

Thanks!

@ntkme ntkme deleted the linux-extra branch December 18, 2023 22:56
@ntkme
Copy link
Contributor Author

ntkme commented Dec 18, 2023

@nex3 Any chance we can cut a release together with the current set of bug fixes, so that we can try it out and fix issues in the release pipeline if any.

jgerigmeyer added a commit to oddbird/embedded-host-node that referenced this pull request Jan 5, 2024
* main:
  Update Dart Sass version and release
  Look for x64 version on arm64 windows (sass#266)
  Update Dart Sass version and release
  Support musl-libc and android (sass#265)
jgerigmeyer added a commit to oddbird/embedded-host-node that referenced this pull request Jan 8, 2024
* main:
  Update Dart Sass version and release
  Look for x64 version on arm64 windows (sass#266)
  Update Dart Sass version and release
  Support musl-libc and android (sass#265)
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 Windows arm64 via the win32-x64 variant
2 participants