Skip to content

Throw a better error when trying to install with musl-libc #149

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

Merged
merged 2 commits into from
Jun 1, 2022
Merged

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented May 31, 2022

No description provided.

@nex3 nex3 requested a review from Goodwine May 31, 2022 22:59
tool/utils.ts Outdated
if (process.platform !== 'linux') return;

const executable = await fs.readFile(process.execPath);
if (!executable.includes('libc.musl-x86_64.so.1')) return;
Copy link
Contributor

@ntkme ntkme May 31, 2022

Choose a reason for hiding this comment

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

We should also consider aarch64 as well as other archs here.

  • libc.musl-x86_64.so.1
  • libc.musl-aarch64.so.1
  • libc.musl-armv7.so.1
  • libc.musl-armhf.so.1
  • etc.

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'm not intimately familiar with what strings those are likely to leave in the executable. Can you give me a list of strings to search for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

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 check against '/lib/ld-musl-' and '/lib/libc.musl-' so it works regardless of architecture?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is extremely rare but multiple ld/libc can be installed in the same system at non-standard locations, so searching with /lib/ might miss some rare cases. If we want good performance in searching, libc.musl- is probably the best.

@ntkme
Copy link
Contributor

ntkme commented May 31, 2022

@nex3 Sorry for going out of topic, but recently CI tests have been failing with either #148 or #150. It would be great if someone from the team can take a look and get those fixes merged. CI tests in dart-sass-embedded is also blocked by the same issues, as JS tests are added recently there.

@nex3 nex3 merged commit 08dd528 into main Jun 1, 2022
@nex3 nex3 deleted the musl-error branch June 1, 2022 01:39
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.

3 participants