Skip to content

fix: run node-gyp rebuild when a project has a binding.gyp file#8325

Merged
zkochan merged 11 commits intopnpm:mainfrom
syi0808:main
Jul 29, 2024
Merged

fix: run node-gyp rebuild when a project has a binding.gyp file#8325
zkochan merged 11 commits intopnpm:mainfrom
syi0808:main

Conversation

@syi0808
Copy link
Copy Markdown
Contributor

@syi0808 syi0808 commented Jul 21, 2024 •
edited
Loading

checkBindingGyp was called only within buildDependency. I don't know exactly, but because of that, checkBindingGyp didn't explore if the chunks weren't circulating inside buildModules.

I've changed this to check unconditionally before install script is executed. But I'm not sure if this is the correct fix. If you give me your opinion, I'll reflect on the test as well.

Fix: #8293

@syi0808 syi0808 requested a review from zkochan as a code owner July 21, 2024 14:54
@welcome
Copy link
Copy Markdown

welcome bot commented Jul 21, 2024

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

@zkochan
Copy link
Copy Markdown
Member

zkochan commented Jul 21, 2024

I believe the fix should happen in the runLifecycleHooksConcurrently function. You can see in that function the scripts is checked but for node-gyp the scripts part is optional. That is why it doesn't work.

@syi0808
Copy link
Copy Markdown
Contributor Author

syi0808 commented Jul 22, 2024

I believe the fix should happen in the runLifecycleHooksConcurrently function. You can see in that function the scripts is checked but for node-gyp the scripts part is optional. That is why it doesn't work.

I've moved this logic into runLifeCycleHooksConcurrently.

@syi0808 syi0808 force-pushed the main branch 2 times, most recently from fa432eb to ac36a0d Compare July 22, 2024 06:00
@syi0808 syi0808 closed this Jul 22, 2024
@syi0808 syi0808 reopened this Jul 22, 2024
@syi0808
Copy link
Copy Markdown
Contributor Author

syi0808 commented Jul 22, 2024

checkBindingGyp is only called inside the buildDependency. regardless of whether there was a module to build in the project or not, I think I should check binding.gyp.

@syi0808
Copy link
Copy Markdown
Contributor Author

syi0808 commented Jul 23, 2024

I modified the test. I also made very small changes to the implementation so that the existing test would not be broken.

@zkochan zkochan changed the title fix(pkg-manager): export checkbindinggyp and check all packages before run install fix: run node-gyp rebuild when a project has a binding.gyp file Jul 28, 2024
@zkochan zkochan merged commit 9899576 into pnpm:main Jul 29, 2024
@welcome
Copy link
Copy Markdown

welcome bot commented Jul 29, 2024

Congrats on merging your first pull request! 🎉🎉🎉

@shirakaba shirakaba mentioned this pull request Apr 25, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zkochan zkochan zkochan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pnpm doesn't default the install script to node-gyp rebuild like npm does on a direct pnpm install of a local project

2 participants

@syi0808 @zkochan