-
Notifications
You must be signed in to change notification settings - Fork 178
feat: add support for force-build-from-source argument #989
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
base: main
Are you sure you want to change the base?
feat: add support for force-build-from-source argument #989
Conversation
ckerr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like exactly what the description says, a simple pass-through to access the --build-from-source flag.
Approving, but since this repo flies under the radar a lot of the time I'm going to hold off for merging for a bit in case @malept or anyone wants to object
malept
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a few things I'd like to see from this:
- if we're going to be adding a flag to force building from source, it shouldn't be limited to
prebuild-install- it should also work forprebuildifyand any other build system we choose to add in the future. In that case I'd prefer if the flag were called--force-build-from-sourceandforceBuildFromSourcerespectively (similar toforceAbi). - There should be tests to verify that we're passing the correct arguments to
prebuild-install, given the value of the build from source flag.
0935bce to
c767e3a
Compare
Codecov Report
@@ Coverage Diff @@
## main #989 +/- ##
==========================================
- Coverage 76.44% 76.12% -0.33%
==========================================
Files 19 20 +1
Lines 692 691 -1
Branches 134 131 -3
==========================================
- Hits 529 526 -3
Misses 118 118
- Partials 45 47 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
c767e3a to
63aef81
Compare
|
Renamed and added a test |
|
Hey folks, I'd like to bump up this PR. @malept can you please take a look? It seems the requested changes were made. Context: I'm looking to migrate the electron-builder package to use electron-rebuild directly (as opposed to the Go-library that handles it), and one of the requirements for us is to have |
BlackHole1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
/cc @malept
|
@mmaietta not sure if this is of any help, but if you're looking for a temporary workaround while this is being approved what we ended up doing in our case is passing a completely fake tag prefix which ensures that prebuilt version cannot be found and forces the build from source to happen |
|
Thanks @gribnoysup ! I'll use that for an alpha release of the npm package |
|
@malept if no objections, can you please merge when you have a chance? 🙂 |
|
Hi @malept , just wanted to remind you of the pending pull request. Many users are waiting for this important update. |
Apparently the changes requested have been made
00235ed to
96adc8f
Compare
|
Rebased on main to resolve conflicts |
Co-authored-by: Erick Zhao <[email protected]>
erickzhao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 179 to 182 in 82da9d9
| if (await moduleRebuilder.prebuildInstallNativeModuleExists()) { | |
| d(`skipping: ${path.basename(modulePath)} as it was prebuilt`); | |
| return; | |
| } |
Hey @gribnoysup, won't this actually short-circuit the rebuild logic before we even call moduleRebuilder.rebuild()?
This patch adds support for the
--build-from-sourceoption of theprebuild-installpackage similar to the--tag-prefixflag from the same tool that is already supported. Sometimes it is helpful to be able to force rebuild for native modules when the electron application is packaged, but currently the--forceflag just makes sure thatprebuild-installis activated, and this tool will prioritize downloading a prebuilt binary if it exists. It might make sense to make--forceimplicitly activate this flag, but to avoid breaking change to the package I added it as a separate flag (but happy to change to whatever the maintainers would prefer)