-
Notifications
You must be signed in to change notification settings - Fork 846
jetpack/install : Print instructions how to resolve composer errors #28285
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
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. |
| stdio, | ||
| } ).catch( err => { | ||
| err.fix = | ||
| 'To resolve issues, try running the command: `cd projects/plugins/jetpack && composer install`'; |
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.
Instead of running composer install yourself, maybe we should suggest running jetpack install xxx just for the element that's causing issues?
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.
The problem is that there is a variety of solutions and the current script is hiding composer error messages.
There is usually a message to run "composer update", but some of the times it wants to downgrade or be run with --ignore-platform-deps, depending on your local setup.
This is meant to at least point you to a right direction, not solve every conceivable problem.
I think in general these scripts try to do too much and are not humble enough to admit when they can't do things for you
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 like @jeherve's suggestion, although I'd recommend including -v to get the verbose output since, unless it was a transient network error or something, it'll probably fail again.
BTW: Running composer update on a project where composer.lock is checked in could cause you problems when submitting the PR, as it might update indirect deps like changelogger's dep on various Symfony packages. Our tools/composer-update-monorepo.sh script is safer as it avoids updating those. If you need --ignore-platform-deps, that probably means your dev environment is screwed up somehow (e.g. a too-old version of PHP).
|
I'll look at this closer today. I'm fine with the idea of this. Is plugins/jetpack hardcoded for a particular reason or is that simply the project you've worked on that has been troubling? Or to phrase another way, is there something particular that installing the jetpack plugin resolves issues elsewhere in the monorepo? Ideally, we'd bubble up the downstream error message. Adding the |
There shouldn't be for the composer install/upgrade. If we don't make any other changes to the message, I'd recommend interpolating the correct directory corresponding to the failed run instead of hard-coding plugins/jetpack.
That might be as simple as changing jetpack/tools/cli/commands/install.js Line 75 in e4fc1ba
to have [ 'ignore', 'ignore', undefined ] for the false case. Although that might be too much in some cases, but getting just the error message and not all the other stuff composer prints would be much less straightforward. Overall I think I like the suggestion to just print a message specifying to run it with -v better.
|
|
This is the bulk of the message I am receiving: Is everyone using |
Yes. We recommend using PHP 8.0 when working in the monorepo. You should see a recommendation to update if you run If you cannot update, you could try using the |
I am updating right now. I also found a small tools called |
|
I have another tool that also uses global PHP version and it requires php7.4. I am really not a fan of global versions TBH. |
If that is the solution, should |
I don't really switch often, but until now I've been happy with multiple versions installed on my machine via brew, and then switching from one to the other with
Yep, it's not a perfect solution; ideally we could rely on the container for everything. Unfortunately that's not always the best option or possible. |
IMO the ONLY downside of docker is that it drains my macbook battery. And being nomad, I care about those things... :) |
When you run
pnpm jetpack/install, it sometimes fails with a cryptic error.The solution is to run the composer update in the appropriate directory, but if you are a newbie in Jetpack, it cant take you days.
This simple change is just injecting a suggestion on how to resolve that when that is needed:
Does this pull request change what data or activity we track or use?
No
Testing instructions
pnpm composer install