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

chore: use @napi-rs/lzma to replace lzma-native #864

Closed
wants to merge 1 commit into from

Conversation

Brooooooklyn
Copy link

See https://github.com/Brooooooklyn/lzma

@napi-rs/lzma support more platform and easier to install (without node-gyp/node-pre-gyp).

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2021

Codecov Report

Merging #864 (2e2d9db) into master (516e74a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
+ Coverage   76.38%   76.41%   +0.03%     
==========================================
  Files          19       19              
  Lines         686      687       +1     
  Branches      131      131              
==========================================
+ Hits          524      525       +1     
  Misses        118      118              
  Partials       44       44              
Impacted Files Coverage Δ
src/sysroot-fetcher.ts 84.84% <100.00%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 516e74a...2e2d9db. Read the comment docs.

@malept
Copy link
Member

malept commented Sep 17, 2021

What platforms are you looking to support that lzma-native doesn't already support with a prebuilt module?

As much as I like Rust, I'm hesitant to add a dependency on a module that could possibly add a dependency on Rust itself.

Also FWIW the readme for lzma-native is a little out of date, it currently uses prebuildify / node-gyp-build to handle the native module building/bundling.

@Brooooooklyn
Copy link
Author

What platforms are you looking to support that lzma-native doesn't already support with a prebuilt module?

Like linux-x64-musl/linux-aarch64-musl, windows-aarch64 and FreeBSD.

As much as I like Rust, I'm hesitant to add a dependency on a module that could possibly add a dependency on Rust itself.

Don't worry about it, you can even use this package in scratch Docker image like node:lts-alpine. It's been fully prebuilt.

@malept
Copy link
Member

malept commented Sep 18, 2021

Like linux-x64-musl/linux-aarch64-musl, windows-aarch64 and FreeBSD.

As far as I can tell, lzma-native supports linux-arm64 and windows-arm64. I'm not particularly concerned about FreeBSD since Electron doesn't officially support it.

@Brooooooklyn
Copy link
Author

lzma-native support linux-x64-gnu/linux-aarch64-gnu but not support linux-x64-musl/linux-aarch64-musl.

But I'm not sure if there were developers use electron on musl linux platform.

@malept
Copy link
Member

malept commented Sep 18, 2021

lzma-native support linux-x64-gnu/linux-aarch64-gnu but not support linux-x64-musl/linux-aarch64-musl.

But I'm not sure if there were developers use electron on musl linux platform.

Electron itself requires glibc, so I doubt it.

@Brooooooklyn
Copy link
Author

And this pull request will absolutely resolve issues like this: addaleax/lzma-native#102

@pbrit
Copy link

pbrit commented Sep 30, 2021

Like linux-x64-musl/linux-aarch64-musl, windows-aarch64 and FreeBSD.

As far as I can tell, lzma-native supports linux-arm64 and windows-arm64. I'm not particularly concerned about FreeBSD since Electron doesn't officially support it.

I believe the above is correct, however, it doesn't support darwin-arm64 which is getting more abundant with advent of Apple's M1 CPU.

This PR did fix my troubles with lzma-native (I'm proud Apple M1 user), and @napi-rs/lzma seems to be a quite neat approach.

@Brooooooklyn @malept Is there anything I can do to get it merged?

Merci @Brooooooklyn !

@malept
Copy link
Member

malept commented Sep 30, 2021

Like linux-x64-musl/linux-aarch64-musl, windows-aarch64 and FreeBSD.

As far as I can tell, lzma-native supports linux-arm64 and windows-arm64. I'm not particularly concerned about FreeBSD since Electron doesn't officially support it.

I believe the above is correct, however, it doesn't support darwin-arm64 which is getting more abundant with advent of Apple's M1 CPU.

This PR did fix my troubles with lzma-native (I'm proud Apple M1 user), and @napi-rs/lzma seems to be a quite neat approach.

@Brooooooklyn @malept Is there anything I can do to get it merged?

Merci @Brooooooklyn !

I asked about darwin-arm64 support in addaleax/lzma-native#118. This PR is on hold until then.

@Brooooooklyn
Copy link
Author

Any progress?

@Brooooooklyn
Copy link
Author

@malept ready to merge again

@beorn
Copy link

beorn commented Jan 5, 2022

This does solve a problem: Currently there doesn't seem to be any way to run electron-forge properly on M1 Macs:

So there are no workarounds.

@electron-bot
Copy link

🎉 This issue has been resolved in version 3.2.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants