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

Integrate Composer into production build process #1836

Closed
wants to merge 3 commits into from

Conversation

felixarntz
Copy link
Collaborator

When running composer install for a production build, it needs to be ensured that all dependencies are compatible with PHP 5.4. It furthermore needs to be ensured that only the dependencies needed for production are part of the build.

This PR integrates Composer into the build process so that it no longer needs to rely on the user manually doing things right in that regard:

  • First, a new shell:composer_production is run, which temporarily removes all local dependencies and installs only the production ones via the --no-dev flag. Furthermore, for that one call the PHP version is hard-set to 5.4, to ensure dependencies are compatible. This mitigates the issue noticed in Switch to installing wp-dev-lib via composer instead of via submodule #1131, where you can't use newer versions of for example the dev dependencies during development.
  • Then, the previously existing shell:webpack_production is run.
  • Then all the files necessary are copied to the build directory. Since vendor now only has the dependencies needed, we can rely on that and copy the whole vendor directory.
  • Afterwards, the regular composer install is run again to have access to all the regular dev dependencies again.
  • Finally, the markdown readme is generated. This uses the dev-lib, which via Switch to installing wp-dev-lib via composer instead of via submodule #1131 will be installed via Composer. Therefore it's crucial for the regular composer install to be run before this, as mentioned above.

@googlebot googlebot added the cla: yes Signed the Google CLA label Jan 21, 2019
@westonruter
Copy link
Member

@kasparsd What do you think of this approach, referencing your #1131 (comment)?

@kasparsd
Copy link

@westonruter I think it is a good idea to lock down the platform requirements and dependency requirements to ensure reliable and reproducible builds by default. Removing the platform requirement from composer.json even with the packages locked down to specific versions has the potential of introducing packages that don't work with PHP 5.4 later down the road (for example, when adding new dependencies).

Considering that we're not planning to remove composer.lock, there is very little point in removing the PHP platform lock since Composer will always install the exact packages specified in the lock file.

How about we keep the defaults locked down as is and introduce a dedicated script that removes those locks when needed -- something like composer_unlock:

composer config --unset platform.php && rm composer.lock

Then composer_production becomes:

composer install --no-dev

which is nice and clean.

Then for the newer versions of PHP in the Travis jobs list we run the unlock command during before_script.

@felixarntz
Copy link
Collaborator Author

Considering that we're not planning to remove composer.lock, there is very little point in removing the PHP platform lock since Composer will always install the exact packages specified in the lock file.

That's a good point. Let's add the platform requirement back to the default composer.json. Given that, I think it's sufficient so entirely rely on that, so we can simplify the composer_production command.

…ser-patches version, and simplify Grunt composer_production logic.
@@ -33,6 +33,12 @@ module.exports = function( grunt ) {
webpack_production: {
command: 'cross-env BABEL_ENV=production webpack'
},
composer_production: {
command: 'rm -rf vendor && composer install --no-dev'
Copy link
Member

Choose a reason for hiding this comment

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

This rm -rf vendor here is problematic because it blows away any Git repos that are installed via composer install --prefer-source. If this is to be done, then it should temporarily rename vendor to vendor-dev and then afterward delete the new vendor and rename vendor-dev back to its original location.

Copy link
Member

Choose a reason for hiding this comment

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

Another solution would be what @kasparsd suggests in #1840.

@@ -94,10 +102,8 @@ module.exports = function( grunt ) {
paths = lsOutput.trim().split( /\n/ ).filter( function( file ) {
return ! /^(blocks|\.|bin|([^/]+)+\.(md|json|xml)|Gruntfile\.js|tests|wp-assets|dev-lib|readme\.md|composer\..*|patches|webpack.*)/.test( file );
} );
paths.push( 'vendor/autoload.php' );
paths.push( 'vendor/**' );
Copy link
Member

Choose a reason for hiding this comment

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

I noticed this is resulting in vendor/cweagans/composer-patches being included unexpectedly. Also, there are lot of php-css-parser files being included which should not be part of the build. In particular, tests. Only the files in vendor/sabberworm/php-css-parser/lib/* should be included.

@westonruter
Copy link
Member

I believe this has been addressed separately. See #3183.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants