-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: binarySource=install #12961
feat: binarySource=install #12961
Conversation
# Conflicts: # lib/util/exec/buildpack.ts # lib/util/exec/index.spec.ts # lib/util/exec/index.ts
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 from code, waiting for docs review. 😉
@viceice testing this locally with:
Getting error:
|
Renovate isn't passing that env. Need to think about how to fallback. 🤔 |
Feel free to push to this branch if any changes are needed here. Sounds like a buildpack change though? |
Yes, needs some buildpack changes. I don't want to force renovate to pass env, which is basically already known |
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.
I think we should hyphenate third-party tools
.
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@viceice I specified USER_NAME and then got this next error:
|
😕 I know the error. Need to fix at buildpack. |
@rarkins can you test agasin with pulling latest renovate image? it has now some changes, so it should work as expected now. |
New problem which is confusing to me. Running:
with binarySource=install in config.js produces an error:
When running "locally" (mac) with binarySource=docker, I instead get:
|
The problem is > yarn why npm
yarn why v1.22.17
[1/4] Why do we have the module "npm"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "npm@7.24.2"
info Reasons this module exists
- "semantic-release#@semantic-release#npm" depends on it
- Hoisted from "semantic-release#@semantic-release#npm#npm"
info Disk size without dependencies: "17.75MB"
info Disk size with unique dependencies: "26.93MB"
info Disk size with transitive dependencies: "32.93MB"
info Number of shared dependencies: 140
Done in 1.22s. |
I think we need to remove local You can run Or we simply remove the bin link at |
Blocked by containerbase/base#220 but otherwise seems to be working. This test repo has three PRs, which were created with npm 6, then 8, then 6: https://github.com/renovate-reproductions/12961/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc Install times for npm are slower than I'd like - about 30 seconds - but I'm assuming that's npm's fault and not ours. The third PR using the same npm 6 as the first one takes only 1 second to return from child process, as expected. |
@viceice I think this is ready for final review/merge now |
🎉 This PR is included in version 30.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Changes:
Adds binarySource=install. Supports npm, composer, jb.
Context:
Closes #11984
Documentation (please check one with an [x])
How I've tested my work (please tick one)
I have verified these changes via: