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

Desktop: Use the strongly recommended asar packing on linux and macOS #2531

Merged
merged 5 commits into from
Feb 26, 2020
Merged

Conversation

slikie
Copy link
Contributor

@slikie slikie commented Feb 20, 2020

No description provided.

R-L-T-Y added 2 commits February 20, 2020 17:30
@laurent22
Copy link
Owner

Does it build?

@slikie
Copy link
Contributor Author

slikie commented Feb 20, 2020

Of course it builds and works fine on my machine.

The CI seems to be not functioning and stuck on the same error for different PR.

@laurent22
Copy link
Owner

laurent22 commented Feb 20, 2020

It used to not work. Since you didn't exclude any of the native modules, did you test that they work? Did you run yarn dist to actually build the executable? on which OS? We don't make massive changes to the build like that with no comment and no information on what tests have been done.

@slikie
Copy link
Contributor Author

slikie commented Feb 20, 2020

I have tried both yarn dist to generate AppImage and electron-builder on latest Arch Linux, no issue either way. With my half a day of usage, it suggest no error on debug mode.

I don't see how I can offer extensive tests for diverse Linux distros.

@tessus
Copy link
Collaborator

tessus commented Feb 20, 2020

I also tested this with macOS (I had to set asar to true for mac) and it seems to work.

@laurent22
Copy link
Owner

Ok it's possible they fixed asar packing then. It's been "strongly recommended" for a while, even when it was obviously broken.

How about resources like icons, fonts and CSS files? Is Katex and Mermaid rendering still working?

@slikie
Copy link
Contributor Author

slikie commented Feb 20, 2020

Resources and all the themes looks fine. Katex works and I don't think Mermaid is in master branch.

@tessus
Copy link
Collaborator

tessus commented Feb 20, 2020

Yes, Katex and Mermaid work too.

@laurent22
Copy link
Owner

Thanks for checking all this @R-L-T-Y and @tessus, it looks like we can use asar then. Please @R-L-T-Y could you fix the conflict? Then we can merge.

@tessus
Copy link
Collaborator

tessus commented Feb 22, 2020

@laurent22 this PR only changes the asar flag for linux.

@tessus tessus added desktop All desktop platforms linux labels Feb 22, 2020
@slikie
Copy link
Contributor Author

slikie commented Feb 23, 2020

Since macOS version has been tested and that should yield a more consistent result, I propose the change made on macOS as well.

@tessus tessus changed the title Desktop: Use the strongly recommended asar packing on linux Desktop: Use the strongly recommended asar packing on linux and macOS Feb 23, 2020
@laurent22
Copy link
Owner

I've also added it to Windows then. I guess it should work too but I'll test later. Thanks for the PR @R-L-T-Y!

@laurent22 laurent22 merged commit ef8af13 into laurent22:master Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms linux macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants