-
Notifications
You must be signed in to change notification settings - Fork 4.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
Env: Support running arbitrary commands and use it for linting and server registered fixtures. #18986
Conversation
…rver registered fixtures.
sources: | ||
- ubuntu-toolchain-r-test | ||
packages: | ||
- libstdc++-4.9-dev |
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.
Can you explain what this is for? Is it possible to leave an inline comment with said explanation?
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.
NodeGit breaks with the default C lib that Travis provides now.
I added a comment.
( err ) => { | ||
spinner.fail( err.message || err.err ); | ||
// eslint-disable-next-line no-console | ||
console.error( `\n\n${ err.out || err.err }\n\n` ); |
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.
Should we prefer the stdout
? Seems like the error message might have more useful contents. Or both, even.
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.
Shouldn't we send errors to stderr
? To avoid unexpected outputs being piped to files and all that?
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.
But if we're handling these errors, I would expect they're not going to be logged to stderr
by default?
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.
They're not, that's why I log them here.
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.
They're not, that's why I log them here.
But if there's both stdout
and stderr
, with this implementation we'd only be logging the former and not the latter?
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.
err.out
and err.err
are both error messages. It's just that some of the used libs format the error object thrown differently.
There are now very frequent Travis build errors due to the use of the custom Example: https://travis-ci.com/WordPress/gutenberg/jobs/269314427
Based on your previous comment at https://github.com/WordPress/gutenberg/pull/18986/files#r358740927 , it's not clear to me what exactly about NodeGit depends on this. Do you know if there is an alternative approach we could take here? Otherwise, I'd be inclined to suggest to revert this pull request to avoid the continued development disruption. |
There are some alternatives in one of the original issues: nodegit/nodegit#853, like building We can also try different Travis Linux distros. |
How viable is it to abandon nodegit and run the native |
Viable, but not trivial. We rely on it for showing download progress, dealing with all the different ref types, and falling back to different checkout strategies for them. |
Follows: #17724
Description
This PR adds support to
@wordpress/env
for running arbitrary commands in one of the underlying docker containers. It also adds 2 containers that will be started on-demand for things like linting, generating fixtures, and PHP unit tests,composer
andchriszarate/wordpress-phpunit
. These are currently used to runnpm run lint-php
andnpm run fixtures:server-registered
.How has this been tested?
It was verified that
npm run lint-php
andnpm run fixtures:server-registered
work as expected.Types of Changes
New Feature:
@wordpress/env
now supports arun
command for running arbitrary commands in one of the underlying Docker containers and also has 2 new containers,composer
andchriszarate/wordpress-phpunit
.Checklist: