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

Prevent packing of native modules #337

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Prevent packing of native modules #337

merged 1 commit into from
Apr 19, 2022

Conversation

PF4Public
Copy link
Contributor

@PF4Public PF4Public commented Apr 12, 2022

Fixes element-hq/element-web#17188

before:

 * Searching for element-desktop ...
 * Contents of net-im/element-desktop-9999:
/usr
/usr/lib64
/usr/lib64/element-desktop
/usr/lib64/element-desktop/app.asar
/usr/lib64/element-desktop/img
/usr/lib64/element-desktop/img/element.ico
/usr/lib64/element-desktop/img/element.png
/usr/lib64/element-desktop/webapp -> ../../share/element-web
/usr/share
/usr/share/applications
/usr/share/applications/electron-16-element-desktop.desktop

after:

 * Searching for element-desktop ...
 * Contents of net-im/element-desktop-1.10.9:
/usr
/usr/lib64
/usr/lib64/element-desktop
/usr/lib64/element-desktop/app.asar
/usr/lib64/element-desktop/app.asar.unpacked
/usr/lib64/element-desktop/app.asar.unpacked/node_modules
/usr/lib64/element-desktop/app.asar.unpacked/node_modules/keytar
/usr/lib64/element-desktop/app.asar.unpacked/node_modules/keytar/build
/usr/lib64/element-desktop/app.asar.unpacked/node_modules/keytar/build/Release
/usr/lib64/element-desktop/app.asar.unpacked/node_modules/keytar/build/Release/keytar.node
/usr/lib64/element-desktop/app.asar.unpacked/node_modules/matrix-seshat
/usr/lib64/element-desktop/app.asar.unpacked/node_modules/matrix-seshat/native
/usr/lib64/element-desktop/app.asar.unpacked/node_modules/matrix-seshat/native/index.node
/usr/lib64/element-desktop/img
/usr/lib64/element-desktop/img/element.ico
/usr/lib64/element-desktop/img/element.png
/usr/lib64/element-desktop/webapp -> ../../share/element-web
/usr/share
/usr/share/applications
/usr/share/applications/electron-18-element-desktop.desktop

Here's what your changelog entry will look like:

🐛 Bug Fixes

@PF4Public PF4Public requested a review from a team as a code owner April 12, 2022 21:32
@ArchangeGabriel
Copy link

Admittedly I was surprised that they managed to pack the native modules into the asar. So not surprised this might not have been a good idea, though I don’t remember why they were packed in the first place.

@PF4Public
Copy link
Contributor Author

I'd guess it is not that natives were intentionally packaged, rather noone bothered not to do so.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

This seems to double up the native modules as now they're both in and out of the asar.

image

@PF4Public
Copy link
Contributor Author

@t3chguy Good catch! I'll look into it.

@PF4Public
Copy link
Contributor Author

@t3chguy Have you tried extracting the resulting asar? I suppose it could be so "by design".

I've done some testing and run this after each rebuild:

node node_modules/.bin/asar l dist/linux-unpacked/resources/app.asar | grep \\.node$;
ls -lh dist/linux-unpacked/resources;
node node_modules/.bin/asar e dist/linux-unpacked/resources/app.asar extrrrrrr;

First without my modifications the result was as follows:

/node_modules/matrix-seshat/native/index.node
/node_modules/keytar/build/Release/keytar.node
total 32M
-rw-r--r-- 1 portage portage  32M Apr 13 22:14 app.asar
drwxr-xr-x 2 portage portage 4.0K Apr 13 22:14 img

After repeating with asarUnpack I got this:

/node_modules/matrix-seshat/native/index.node
/node_modules/keytar/build/Release/keytar.node
total 25M
-rw-r--r-- 1 portage portage  25M Apr 13 22:19 app.asar
drwxr-xr-x 3 portage portage 4.0K Apr 13 22:19 app.asar.unpacked
drwxr-xr-x 2 portage portage 4.0K Apr 13 22:19 img

It is obviously lighter now. Natives could still be referenced in asar, but absent from it. Lets check this assumption by renaming app.asar.unpacked:

node node_modules/.bin/asar l dist/linux-unpacked/resources/app.asar | grep \\.node$;
mv dist/linux-unpacked/resources/app.asar.unpacked dist/linux-unpacked/resources/dhdfghsetrhsrthjdrthdrtj
ls -lh dist/linux-unpacked/resources;
node node_modules/.bin/asar e dist/linux-unpacked/resources/app.asar extrrrrrr;

It fails with a very descriptive error message :)

/node_modules/matrix-seshat/native/index.node
/node_modules/keytar/build/Release/keytar.node
total 25M
-rw-r--r-- 1 portage portage  25M Apr 13 22:22 app.asar
drwxr-xr-x 3 portage portage 4.0K Apr 13 22:22 dhdfghsetrhsrthjdrthdrtj
drwxr-xr-x 2 portage portage 4.0K Apr 13 22:22 img
internal/fs/utils.js:314
    throw err;
    ^

Error: ENOENT: no such file or directory, open '…snip…/dist/linux-unpacked/resources/app.asar.unpacked/node_modules/matrix-seshat/native/index.node'

Another interesting thing. I found this in electron-builder docs:

Node modules, that must be unpacked, will be detected automatically, you don’t need to explicitly set asarUnpack - please file an issue if this doesn’t work.

This suggests it could be also a bug in electron-builder 🤷

@t3chguy
Copy link
Member

t3chguy commented Apr 13, 2022

Thanks for the deep dive, can confirm this works and is a great one liner! Thanks

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

@PF4Public
Copy link
Contributor Author

If "legally identifiable name" includes "pseudonym", I would be happy to rewrite the commit and add Signed-off-by:.

@turt2live
Copy link
Member

If it's legal where you're at, then yes.

@ArchangeGabriel
Copy link

Well given the point is that it cannot be anonymous, I don’t think that would work. What should be done in this case? Just someone else pushing the same change?

Signed-off-by: PF4Public <PF4Public@users.noreply.github.com>
@turt2live
Copy link
Member

@ArchangeGabriel we've accepted contributions from pseudonyms before, but are aware of several countries where it wouldn't be legally binding.

@turt2live turt2live requested a review from t3chguy April 13, 2022 20:15
@PF4Public
Copy link
Contributor Author

@t3chguy @turt2live Done.

Well given the point is that it cannot be anonymous, I don’t think that would work.

Pseudonymous is not anonymous.

What should be done in this case? Just someone else pushing the same change?

Probbaly this:

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

@robintown
Copy link
Member

@PF4Public If you don't want to disclose your legal name publicly, another option is to email dco@matrix.org, who have a process in place for this kind of thing.

@PF4Public
Copy link
Contributor Author

@PF4Public If you don't want to disclose your legal name publicly, another option is to email dco@matrix.org, who have a process in place for this kind of thing.

Thanks for suggestion! But I didn't contribute any significant mount of code, so I assume pseudonym suffice.

@ArchangeGabriel
Copy link

ArchangeGabriel commented Apr 13, 2022

So we discussed that OOB, and pseudonymous is fine enough to not be anonymous here (I said that this would be really context dependent, because for all that matters a pseudonym could be completely untraceable to a physical person), especially given the small amount of change as stated.

@PF4Public
Copy link
Contributor Author

pseudonym could be completely untraceable to a physical person

If I were to sign as follows: Signed-off-by: Nashorn Unpaarhuferson <NaUR@Upplýsingalæsi.Skuespiller.示例域> how am i to be traced?

@turt2live
Copy link
Member

we're having this conversation elsewhere - please let's keep it off the PR.

We don't technically require signoff on this change, but it's borderline enough where it's warranted to ask for it. If we have any reason to believe this change was made in bad faith (stolen code, malicious intent, etc) then we'd handle it accordingly.

@PF4Public
Copy link
Contributor Author

Whatever you decide. Feel free to submit this one-liner in the name of one of your developers. No problem. Just fix this year-long nuisance already :)

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

:shipit:

@t3chguy t3chguy merged commit 961a6f4 into element-hq:develop Apr 19, 2022
su-ex added a commit to SchildiChat/element-desktop that referenced this pull request Apr 30, 2022
* Handle forced disconnects from Jitsi ([\#21697](element-hq/element-web#21697)). Fixes element-hq/element-web#21517.
* Improve performance of switching to rooms with lots of servers and ACLs ([\#8347](matrix-org/matrix-react-sdk#8347)).
* Avoid a reflow when setting caret position on an empty composer ([\#8348](matrix-org/matrix-react-sdk#8348)).
* Add message right-click context menu as a labs feature ([\#5672](matrix-org/matrix-react-sdk#5672)).
* Live location sharing - basic maximised beacon map ([\#8310](matrix-org/matrix-react-sdk#8310)).
* Live location sharing - render users own beacons in timeline ([\#8296](matrix-org/matrix-react-sdk#8296)).
* Improve Threads beta around degraded mode ([\#8318](matrix-org/matrix-react-sdk#8318)).
* Live location sharing -  beacon in timeline happy path ([\#8285](matrix-org/matrix-react-sdk#8285)).
* Add copy button to View Source screen ([\#8278](matrix-org/matrix-react-sdk#8278)). Fixes element-hq/element-web#21482. Contributed by @olivialivia.
* Add heart effect ([\#6188](matrix-org/matrix-react-sdk#6188)). Contributed by @CicadaCinema.
* Update new room icon ([\#8239](matrix-org/matrix-react-sdk#8239)).
* Prevent packing of native modules ([\element-hq#337](element-hq#337)). Fixes element-hq/element-web#17188. Contributed by @PF4Public.
* Fix: "Code formatting button does not escape backticks" ([\#8181](matrix-org/matrix-react-sdk#8181)). Contributed by @yaya-usman.
* Fix beta indicator dot causing excessive CPU usage ([\#8340](matrix-org/matrix-react-sdk#8340)). Fixes element-hq/element-web#21793.
* Fix overlapping timestamps on empty messages ([\#8205](matrix-org/matrix-react-sdk#8205)). Fixes element-hq/element-web#21381. Contributed by @goelesha.
* Fix power selector not showing up in user info when state_default undefined ([\#8297](matrix-org/matrix-react-sdk#8297)). Fixes element-hq/element-web#21669.
* Avoid looking up settings during timeline rendering ([\#8313](matrix-org/matrix-react-sdk#8313)). Fixes element-hq/element-web#21740.
* Fix a soft crash with video rooms ([\#8333](matrix-org/matrix-react-sdk#8333)).
* Fixes call tiles overflow ([\#8096](matrix-org/matrix-react-sdk#8096)). Fixes element-hq/element-web#20254. Contributed by @luixxiul.
* Fix a bug with emoji autocomplete sorting where adding the final "&element-hq#58;" would cause the emoji with the typed shortcode to no longer be at the top of the autocomplete list. ([\#8086](matrix-org/matrix-react-sdk#8086)). Fixes element-hq/element-web#19302. Contributed by @commonlawfeature.
* Fix image preview sizing for edge cases ([\#8322](matrix-org/matrix-react-sdk#8322)). Fixes element-hq/element-web#20088.
* Refactor SecurityRoomSettingsTab and remove unused state ([\#8306](matrix-org/matrix-react-sdk#8306)). Fixes matrix-org/element-web-rageshakes#12002.
* Don't show the prompt to enable desktop notifications immediately after registration ([\#8274](matrix-org/matrix-react-sdk#8274)).
* Stop tracking threads if threads support is disabled ([\#8308](matrix-org/matrix-react-sdk#8308)). Fixes element-hq/element-web#21766.
* Fix some issues with threads rendering ([\#8305](matrix-org/matrix-react-sdk#8305)). Fixes element-hq/element-web#21670.
* Fix threads rendering issue in Safari ([\#8298](matrix-org/matrix-react-sdk#8298)). Fixes element-hq/element-web#21757.
* Fix space panel width change on hovering over space item ([\#8299](matrix-org/matrix-react-sdk#8299)). Fixes element-hq/element-web#19891.
* Hide the reply in thread button in deployments where beta is forcibly disabled ([\#8294](matrix-org/matrix-react-sdk#8294)). Fixes element-hq/element-web#21753.
* Prevent soft crash around room list header context menu when space changes ([\#8289](matrix-org/matrix-react-sdk#8289)). Fixes matrix-org/element-web-rageshakes#11416, matrix-org/element-web-rageshakes#11692, matrix-org/element-web-rageshakes#11739, matrix-org/element-web-rageshakes#11772, matrix-org/element-web-rageshakes#11891 matrix-org/element-web-rageshakes#11858 and matrix-org/element-web-rageshakes#11456.
* When selecting reply in thread on a thread response open existing thread ([\#8291](matrix-org/matrix-react-sdk#8291)). Fixes element-hq/element-web#21743.
* Handle thread bundled relationships coming from the server via MSC3666 ([\#8292](matrix-org/matrix-react-sdk#8292)). Fixes element-hq/element-web#21450.
* Fix: Avatar preview does not update when same file is selected repeatedly ([\#8288](matrix-org/matrix-react-sdk#8288)). Fixes element-hq/element-web#20098.
* Fix a bug where user gets a warning when changing powerlevel from **Admin** to **custom level (100)** ([\#8248](matrix-org/matrix-react-sdk#8248)). Fixes element-hq/element-web#21682. Contributed by @Jumeb.
* Use a consistent alignment for all text items in a list ([\#8276](matrix-org/matrix-react-sdk#8276)). Fixes element-hq/element-web#21731. Contributed by @luixxiul.
* Fixes button labels being collapsed per a character in CJK languages ([\#8212](matrix-org/matrix-react-sdk#8212)). Fixes element-hq/element-web#21287. Contributed by @luixxiul.
* Fix: Remove jittery timeline scrolling after jumping to an event ([\#8263](matrix-org/matrix-react-sdk#8263)).
* Fix regression of edits showing up in the timeline with hidden events shown ([\#8260](matrix-org/matrix-react-sdk#8260)). Fixes element-hq/element-web#21694.
* Fix reporting events not working ([\#8257](matrix-org/matrix-react-sdk#8257)). Fixes element-hq/element-web#21713.
* Make Jitsi widgets in video rooms immutable ([\#8244](matrix-org/matrix-react-sdk#8244)). Fixes element-hq/element-web#21647.
* Fix: Ensure links to events scroll the correct events into view ([\#8250](matrix-org/matrix-react-sdk#8250)). Fixes element-hq/element-web#19934.
zhuyifei1999 referenced this pull request in gentoo/guru Jun 17, 2023
Signed-off-by: YiFei Zhu <zhuyifei1999@gmail.com>
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.

element-desktop creates /tmp/.org.chromium.Chromium.xxxxxx files when started, which are not deleted upon exit
5 participants