Skip to content

feat: add support for force-build-from-source argument - #989

Open
gribnoysup wants to merge 3 commits into
electron:mainfrom
gribnoysup:add-support-for-build-from-source
Open

feat: add support for force-build-from-source argument#989
gribnoysup wants to merge 3 commits into
electron:mainfrom
gribnoysup:add-support-for-build-from-source

Conversation

@gribnoysup

Copy link
Copy Markdown

This patch adds support for the --build-from-source option of the prebuild-install package similar to the --tag-prefix flag 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 --force flag just makes sure that prebuild-install is activated, and this tool will prioritize downloading a prebuilt binary if it exists. It might make sense to make --force implicitly 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)

@ckerr ckerr left a comment

Copy link
Copy Markdown
Member

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
malept previously requested changes May 17, 2022

@malept malept left a comment

Copy link
Copy Markdown
Member

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 for prebuildify and 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-source and forceBuildFromSource respectively (similar to forceAbi).
  • 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.

@gribnoysup
gribnoysup force-pushed the add-support-for-build-from-source branch from 0935bce to c767e3a Compare May 28, 2022 13:45
@codecov-commenter

codecov-commenter commented May 28, 2022

Copy link
Copy Markdown

Codecov Report

Merging #989 (7af98bb) into main (29365ad) will decrease coverage by 0.33%.
The diff coverage is 79.62%.

@@            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     
Impacted Files Coverage Δ
src/clang-fetcher.ts 81.42% <ø> (+0.34%) ⬆️
src/types.ts 100.00% <ø> (ø)
src/module-type/node-gyp/node-gyp.ts 83.87% <75.00%> (ø)
src/module-type/node-gyp/worker.ts 76.19% <76.19%> (ø)
src/module-rebuilder.ts 91.52% <100.00%> (ø)
src/module-type/prebuild-install.ts 90.00% <100.00%> (ø)
src/rebuild.ts 72.82% <100.00%> (+0.29%) ⬆️
src/sysroot-fetcher.ts 83.87% <100.00%> (-0.51%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gribnoysup
gribnoysup force-pushed the add-support-for-build-from-source branch from c767e3a to 63aef81 Compare May 28, 2022 14:13
@gribnoysup

Copy link
Copy Markdown
Author

Renamed and added a test

Comment thread test/module-type-prebuild-install.ts Outdated
@mmaietta

Copy link
Copy Markdown
Contributor

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 forceBuildFromSource for us to cleanly transition

@BlackHole1 BlackHole1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.
/cc @malept

@gribnoysup

Copy link
Copy Markdown
Author

@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

@mmaietta

Copy link
Copy Markdown
Contributor

Thanks @gribnoysup ! I'll use that for an alpha release of the npm package electron-builder. However, I don't think it should be the long-term implementation I use, so I'll still need this PR merged/released before I can proceed with a "prod" RC

@mmaietta

Copy link
Copy Markdown
Contributor

@malept if no objections, can you please merge when you have a chance? 🙂

@gaodeng

gaodeng commented Jun 2, 2023

Copy link
Copy Markdown

Hi @malept , just wanted to remind you of the pending pull request. Many users are waiting for this important update.

@malept
malept dismissed their stale review June 2, 2023 15:47

Apparently the changes requested have been made

@gribnoysup
gribnoysup force-pushed the add-support-for-build-from-source branch from 00235ed to 96adc8f Compare June 3, 2023 17:24
@gribnoysup
gribnoysup requested a review from a team as a code owner June 3, 2023 17:24
@gribnoysup

Copy link
Copy Markdown
Author

Rebased on main to resolve conflicts

@gribnoysup gribnoysup changed the title feat: Add support for build-from-source argument feat: add support for force-build-from-source argument Jun 5, 2023
@erickzhao
erickzhao self-requested a review June 5, 2023 18:46
Comment thread README.md Outdated
Co-authored-by: Erick Zhao <erick@hotmail.ca>

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

rebuild/src/rebuild.ts

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()?

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.

8 participants