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

fix: yarn install failed when dependency package using yarn in postin… #6350

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benny1213
Copy link

@benny1213 benny1213 commented Jun 20, 2024

What's the problem this PR addresses?

when running yarn install script, there is an error occur:
"xx must be built because it never has been before or the last one failed" and the postinstall script of the error package did not run correctly.

Preconditions:

  1. using yarnPath with a pre-built yarn.js
  2. some of our npm package include yarn command in the postinstall script.

you can find more detail from minimal reproducible repo

How did you fix it?

after debug, i found the linkersCustomData did not restore correctly when running the yarn in the postinstall script, which causing yarn command in postinstall script refused to run.

image

so i tried calling project.restoreInstallState() when customData is missing to try restore the customData.

after i fixed this issue, i found two little issue still showing:

  1. when calling project.findLocatorForLocation yarn alway pass a path with a / ending, but the locatorByPath map only contains key without / ending.
  2. the locator should be a Locator struct instead of a string
image

i also fixed these two problem as the file changes show.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@benny1213 benny1213 force-pushed the jiahan/fix-postinstall-script-using-yarn-issue branch 4 times, most recently from cc5789e to 8b95300 Compare June 21, 2024 03:34
@arcanis
Copy link
Member

arcanis commented Jun 21, 2024

Can you add a comprehensive description of what this change is supposed to do/fix?

@benny1213
Copy link
Author

Can you add a comprehensive description of what this change is supposed to do/fix?

sure, i will create a minimal Minimal reproducible repository for better describe this PR later, please wait a moment thanks.

…stall

fix: lint issue

fix: package cwd slash issue

fix: lint issue
@benny1213 benny1213 force-pushed the jiahan/fix-postinstall-script-using-yarn-issue branch from 57c48f8 to 856b682 Compare June 21, 2024 13:10
@benny1213
Copy link
Author

hi @arcanis it's my first time raising a PR to public repo, If there is anything wrong, I am willing to modify it.

i think there is two work still need to do:

  1. should write a test for this case, but i don't know how for this error case, can anyone help advice?
  2. i can't pass all the test pipeline, it showing certification issue in windows node pipeline, i think it's not relevant to my change.

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.

2 participants