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

Make a setting for --build-from-option flag #790

Merged
merged 4 commits into from
Oct 6, 2016
Merged

Make a setting for --build-from-option flag #790

merged 4 commits into from
Oct 6, 2016

Conversation

0181532686cf4a31163be0bf3e6bb6732bf
Copy link
Contributor

This seems to fix #787

The only place I'm unsure of is (haven't touched yet):

await installDependencies(results[0], results[1], args.arch)

@mention-bot
Copy link

@lyssdod, thanks for your PR! By analyzing the history of the files in this pull request, we identified @develar, @demetris-manikas and @symbyte to be potential reviewers.

@0181532686cf4a31163be0bf3e6bb6732bf
Copy link
Contributor Author

My main concern for this — I have to create installers for all platforms, but I have only one (Mac) available. Since I won't be able to rebuild my native modules with MSVC on Mac, I would like to use already present precompiled binaries for bundling

@darkshades42
Copy link

Hi! Any word on a review here? @develar

@develar
Copy link
Member

develar commented Oct 5, 2016

Did you investigate why #647 doesn't work? I mean — why " it uses prebuilt to download native binary instead of compiling."? Why incorrect binaries were used?

I don't want to introduce yet another flag, it is better to investigate — is it really bug in the precompiled binaries.

@0181532686cf4a31163be0bf3e6bb6732bf
Copy link
Contributor Author

@develar I`ll try to investigate 5.22.2 release

@develar
Copy link
Member

develar commented Oct 5, 2016

@lyssdod Issue is not in the electron-builder, should be reproducible using latest version also.

@0181532686cf4a31163be0bf3e6bb6732bf
Copy link
Contributor Author

The trick is in the --build-from-source flag which's being handled by node-pre-gyp and forces compilation instead of checking for binaries. In my particular case compilation fails because I've moved all compilation deps to devDependencies to minimize final package size (for fast npm install which will install only needed stuff). Moving these deps back to dependencies eliminates the benefits of using node-pre-gyp completely, because this way it will compile the code for one particular platform (host platform electron-builder is running at), but not for others.

@develar
Copy link
Member

develar commented Oct 5, 2016

@lyssdod I understand you completely. But I want to understand — does node-pre-gyp works correctly or not. Bug #647 was reproduced by me — incorrect precompiled binaries were used. It is very, very critical — because you cannot test your app on all platforms and archs every release.

So, I ask you to check — is it node-pre-gyp bug or not. Who is guilty :) To ensure, that your approach really works and correct binaries will be used in ALL cases.

If you don't want to do this work, I can accept PR but this flag will be not documented and marked as experimental. And not by default.

@0181532686cf4a31163be0bf3e6bb6732bf
Copy link
Contributor Author

I'm all up for solving this issue on the node-pre-gyp's side, it just seems to me that they're not very active and responsive to issues (in contrast to you 👍 ): mapbox/node-pre-gyp#239

I`ll look into bug #647 more thoroughly now.

@0181532686cf4a31163be0bf3e6bb6732bf
Copy link
Contributor Author

0181532686cf4a31163be0bf3e6bb6732bf commented Oct 5, 2016

Hi again @develar,

I've looked into #647 but didn't see any useful info there (release used there already has this flag enabled). Instead I've prepared a test repo which uses prebuilt modules after patching electron-builder:

https://github.com/lyssdod/electron-abi-test

I don't want to introduce yet another flag

http://electron.atom.io/docs/tutorial/using-native-node-modules/ says there's another method of specifying it:

# Tell node-pre-gyp to build module from source code.
export npm_config_build_from_source=true

I can update PR to handle this option instead of handling the flag if it makes sense.

Relevant lines in node-pre-gyp: https://github.com/mapbox/node-pre-gyp/blob/master/lib/install.js#L125-L132

@develar
Copy link
Member

develar commented Oct 6, 2016

@lyssdod Ok, thanks.

So in general it is recommended to always build native modules from source code.

For now this flag will be not enabled by default, according to this note.

@develar develar merged commit 4898eb1 into electron-userland:master Oct 6, 2016
@0181532686cf4a31163be0bf3e6bb6732bf
Copy link
Contributor Author

Thank you!

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.

Install app deps always trigger rebuild
4 participants