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

feat: add support for node-gyp --jobs option to parallelize an indivi… #1106

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nkallen
Copy link

@nkallen nkallen commented Oct 1, 2023

Hi,

I have a native module with a couple hundred c++ files. Electron-rebuild does have a "parallel" option but this doesn't actually run make in parallel WITHIN one module. This adds support for node-gyp's --jobs option, so that users like me can run electron-rebuild --jobs=max from the command line

Thanks

@nkallen nkallen requested a review from a team as a code owner October 1, 2023 20:58
@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2023

Codecov Report

Merging #1106 (cbed90e) into main (bed3539) will increase coverage by 0.03%.
Report is 5 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1106      +/-   ##
==========================================
+ Coverage   76.30%   76.34%   +0.03%     
==========================================
  Files          21       21              
  Lines         726      727       +1     
  Branches      136      137       +1     
==========================================
+ Hits          554      555       +1     
  Misses        121      121              
  Partials       51       51              
Files Coverage Δ
src/module-type/node-gyp/node-gyp.ts 83.87% <ø> (ø)
src/rebuild.ts 73.11% <100.00%> (+0.29%) ⬆️
src/types.ts 100.00% <ø> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@nkallen
Copy link
Author

nkallen commented Oct 12, 2023

Hi,

Are you guys not accepting submissions? I'm pretty
Sure this is a useful feature for everyone

Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM. Although it can be achieved through the JOBS environment variable in node-gyp, I personally feel that supporting the --jobs parameter would be better. Thank you for your contribution!

PTAL @dsanders11

@dsanders11 dsanders11 self-assigned this Nov 7, 2023
@erickzhao
Copy link
Member

ref #303

@dsanders11
Copy link
Member

References #303.

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

Small docs change, but LGTM

README.md Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

I think this may be currently possible by setting the npm_config_jobs environment variable, see the node-gyp documentation.

@nkallen, could you test using the environment variable to change the number of jobs and see if that works for your use case? If it does, I'd rather document in this repo that you can pass additional options to node-gyp in this manner, rather than plumbing in through. Someone may come along with the need for another node-gyp option, like --python, and plumbing them through on a case-by-case basis doesn't seem ideal if they're easy to set via environment variables.

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.

5 participants