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

Build Tools: Install Composer dependencies as pre-lint step #21537

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 10, 2020

Related:

This pull request seeks to install Composer dependencies as part of the PHP lint script. This is intended to improve the developer experience, where a default installation of the development environment would not allow for npm run lint-php to be run. This script assumes Composer dependencies to already have been installed. Unless the contributor explicitly installs these dependencies via the (undocumented) npm run wp-env run composer install command, they cannot run the tests locally.

These changes bring uniformity to how lint testing occurs between a contributor's environment and in Travis. Previously, our Travis environment was "privileged" to have these setup tasks run, but these setup tasks are not made available to standard contributors, nor are they documented.

Implementation Notes:

Very important to note: This changes Composer installation in Travis from using the (legacy?) "env" setup, to the new "wp-env" setup. It's most important in considering the potential impact of reintroducing the issue intended to have been addressed in #21118. Fortunately, it should be relatively obvious if there are any issues, since the Travis build would pass only if working correctly.

As illustrated by the above-referenced Slack discussion, I'm not particularly attached to this approach, but I do fear the developer experience is currently very lacking. It took me quite some time to figure out how to run PHP linting successfully. While this implementation can be "wasteful" in that it runs the composer install step even if dependencies are already installed, an alternative was not clear in how to reduce friction for contributors to run tests.

It should be relatively safe that this only applies to linting, since all of the Composer dependencies are specific to linting (source).

Possible alternatives could include:

  • Put responsibility on the developer to install Composer dependencies as part of preparing their local environment, not much unlike an expectation that a developer should be running npm install before starting development work.
    • This must be clearly documented, and it is not currently.
  • Something in the implementation of the wp-env scripts could infer based on an incoming command whether it should need to prepare the environment to be able to run the command.
  • Add messaging to the failed test run if dependencies are absent, instructing the developer actions to take before trying again.
    • There is some prior art to this with the npm run test-php script:

Please ensure the WP_DEVELOP_DIR environment variable is set to your WordPress Development directory before running this script.

If you don't have a WordPress Development directory to use, run npm run env install to automatically configure one!

Testing Instructions:

  1. Prepare a fresh development environment, following the "Getting Started" guide.
  2. Run npm run lint-php.
  3. Observe that it passes successfully.

@aduth aduth added the [Type] Build Tooling Issues or PRs related to build tooling label Apr 10, 2020
@aduth aduth requested a review from noisysocks April 10, 2020 18:56
@github-actions
Copy link

Size Change: 0 B

Total Size: 903 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 104 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.15 kB 0 B
build/block-library/style.css 7.16 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.6 kB 0 B
build/components/style.css 16.5 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.1 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/index.js 93.5 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.4 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.53 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.6 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.29 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this.

@aduth
Copy link
Member Author

aduth commented Apr 15, 2020

Noting that there's some related work happening in #20280 and #20090, at least regarding making the environment consistent between Travis and local development, in order to avoid these weird developer experience barriers of getting the tests to run the same way they do in continuous integration. That said, even as of those changes, the INSTALL_COMPOSER step removed here would still have been left. I think there may still be some alternatives to consider. For me, ideally these occur both transparently and on-demand, either as part of the local environment setup process, or the test run itself. "As part of the test run itself" does happen with these changes, though more wastefully than is often necessary (because it is running composer install every test run).

@aduth aduth merged commit 078c83b into master Apr 17, 2020
@aduth aduth deleted the fix/local-php-lint branch April 17, 2020 20:02
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 17, 2020
@noahtallen
Copy link
Member

Hm. This doesn't seem to be working for me locally 🤔

~/source/gutenberg(master) » npm run wp-env run composer install -- --no-interaction                                                                                                                                                                         noah.allen@Noahs-MacBook-Pro

> gutenberg@8.1.0-rc.1 wp-env /Users/noah.allen/source/gutenberg
> packages/env/bin/wp-env "run" "composer" "install" "--no-interaction"

⠹ Running `install` in 'composer'.

Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files



⠋ wp-env run <container> [command..]

Runs an arbitrary command in one of the underlying Docker containers, for
example it's useful for running wp cli commands.

Positionals:
  container  The container to run the command on.            [string] [required]
  command    The command to run.                           [array] [default: []]

Options:
  --help     Show help                                                 [boolean]
  --version  Show version number                                       [boolean]
  --debug    Enable debug output.                     [boolean] [default: false]

TypeError: Cannot use 'in' operator to search for 'exitCode' in Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files

    at /Users/noah.allen/source/gutenberg/packages/env/lib/cli.js:43:16
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! gutenberg@8.1.0-rc.1 wp-env: `packages/env/bin/wp-env "run" "composer" "install" "--no-interaction"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the gutenberg@8.1.0-rc.1 wp-env script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/noah.allen/.npm/_logs/2020-05-12T21_21_18_745Z-debug.log

@noahtallen
Copy link
Member

It still fails just running ./packages/env/bin/wp-env run composer install 🤔 🤔

@aduth
Copy link
Member Author

aduth commented May 12, 2020

@noahtallen Yeah, I've seen some similar issues with this lately. Which is odd, because as of these changes, Travis runs the lint through the exact same process.

Do you get the same error when running ./packages/env/bin/wp-env run composer install ?

@noahtallen
Copy link
Member

Yep. It looks like docker compose includes a string as the err property of the response, and then wp-env tries to see if there is an error code in that. which doesn't work obviously.

I'll see what happens when we check to make sure that err is an object

@noahtallen
Copy link
Member

If we do that:

✖ Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files

Looks a lot nicer, but it doesn't actually seem to be an error!

@noahtallen noahtallen mentioned this pull request May 13, 2020
8 tasks
@aduth
Copy link
Member Author

aduth commented May 13, 2020

Do you think the problem is that the presence of any output is being interpreted as an error? Even if it's not an error?

@noahtallen
Copy link
Member

It works correctly for other commands, so I'm not sure. I think err and out correspond to stderr/stdout in the container. So maybe composer is doing something unexpected?

@noahtallen
Copy link
Member

I think this might just error out when there is nothing to install or update. It probably passes in CI since it does have stuff to install and update

@noahtallen
Copy link
Member

this should fix it: #22475

noahtallen added a commit that referenced this pull request May 19, 2020
@aduth
Copy link
Member Author

aduth commented May 20, 2020

For what it's worth, I'm not entirely loving the changes that were introduced here. At least in common usage, the composer install step adds significant time to what should otherwise be a pretty quick lint command. It's certainly nice to have to avoid confusion for contributors who haven't yet gone through the process of having the composer dependencies prepared yet. But after that initial setup, the direct command is much faster. I've found that I've been tending to run the direct command npm run wp-env run composer run-script lint, which could partly explain why I wasn't encountering the errors.

I'm still not sure what the best solution here would be. It's a bit similar to needing to run npm install occasionally to get or update local dependencies. My experience was that it wasn't as obvious to do this for composer dependencies. Perhaps others encounter the same confusion with npm. It's not like we run npm install as a pre-step to all the Node-based scripts 🤷

@noahtallen
Copy link
Member

I agree with you here. It makes no sense why composer takes 2-3 minutes to install when everything is already up to date. Maybe we could expose a package script to run composer install? At that point I think it’s fine to tell people they need to run it once before doing phpcs or phpunit

@aduth
Copy link
Member Author

aduth commented May 20, 2020

At that point I think it’s fine to tell people they need to run it once before doing phpcs or phpunit

I do think it's a simple option, but really only if it can be communicated effectively in a way which won't just be utterly confusing.

I haven't looked into it, but one option could be to be more helpful in the error case, if there's some way that the failures associated with not having run this step can include a tip on what the developer needs to do to fix it.

Example:

It appears that the Composer dependencies have not been installed. Run npm run composer-install, then try again.

@noahtallen
Copy link
Member

Yeah that would be really useful. Part of me thinks it would be really useful for wp-env too since phpcs and phpunit are very common use cases, but I'm not sure how to integrate those more closely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants