Running `build` scripts on Windows machines produces different results

[desrosj]

When running npm run build:dev on a Windows machine, the hashes calculated by Webpack are different and not as expected.

This seems to be related to an encoding issue with how the remove-accents dependency is read and compiled.

Slack discussion thread: ​https://wordpress.slack.com/archives/C02RQBWTW/p1663687669117549?thread_ts=1663687642.461539&cid=C02RQBWTW

[desrosj]

In 54277:

Build/Test Tools: Temporarily allow the NPM testing workflow to fail.

This changes the Test NPM GitHub Action workflow to temporarily allow the job validating build tools on Windows to fail.

When the build:dev Grunt task is run on Windows, the resulting hashes calculated by Webpack are different than other platforms. This seems to be related to how the remove-accents dependency is read and processed during the build script.

Since a Windows machine is not used to build WordPress on the build server, this will only affect local development installs for Windows contributors. While this is investigated, this workflow job can be allowed to fail.

Props bernhard-reiter, desrosj, Clorith, gziolo.
See #56615.

[desrosj]

Related: #55616.

Trac ticket: https://core.trac.wordpress.org/ticket/56615

It's based on the finding from @jsnajdr that stable hashes are only guaranteed for the production version of the build.

I did some more debugging today and it seems that not even production build reliably guarantees stable order of modules and stable hashes. When the chunk modules are written to a file, they are ordered by module.identifier(), which is not the thing that you defined in optimization.moduleIds, e.g., named or deterministic, but the module's request field, which is the full absolute path to the module, like /Users/jsnajdr/project/src/index.js. See this webpack method that returns this.request and doesn't really look at anything else:

​https://github.com/webpack/webpack/blob/9fcaa243573005d6fdece9a3f8d89a0e8b399613/lib/NormalModule.js#L338-L349

The real reason why production builds are OK is that production uses module concatenation. The offending date-fns modules are concatenated into another module and are no longer written separately. Then the unstable sorting doesn't happen.

I'll be investigating further in cooperation with @ockham

I'll be investigating further in cooperation with @ockham

Thank you so much for sharing your findings. That would be great to confirm it won't cause similar issues in the future.

In the meantime, I applied all the necessary changes to make CI happy. I think it's a good improvement on its own as it allows us to keep only the file with combined assets for production to ensure that they never differ between platforms.

I'll be investigating further in cooperation with @ockham

Thank you so much for sharing your findings. That would be great to confirm it won't cause similar issues in the future.

In the meantime, I applied all the necessary changes to make CI happy. I think it's a good improvement on its own as it allows us to keep only the file with combined assets for production to ensure that they never differ between platforms.

[gziolo]

It would be great to test the changes proposed with a fresh repository on Windows and Unix to ensure that unit tests pass.

I'll be investigating further in cooperation with @ockham

Thank you so much for sharing your findings. That would be great to confirm it won't cause similar issues in the future.

Update: @jsnajdr prepared a minimal test case that I ran on Windows. He'll use the results to file a bug report against Webpack.

Webpack bug reported here: ​https://github.com/webpack/webpack/issues/16289

[gziolo]

In 54289:

Build: Improve how combined assets are generated

Allows to revert changes applied in [54277] - temporary workaround for the failing Test NPM CI check on Windows.

Improvements included:

• generate combined asset files for both production and development
• store in the repository only the production version of the combined assets for packages, we use everything else only in development
• to make unit tests work, ensure that they ignore react fast refresh and use the production version of combined assets that are present in the source code

Props bernhard-reiter, jsnajdr, clorith, wildworks.
Fixes #56615.

Committed with https://core.trac.wordpress.org/changeset/54289.

[jrf]

Eh... I'm not sure what you all were trying to do here, but commit [54289] breaks ​three out of the four previously supported test workflows.

I won't bore you with the full error log, but the short version comes down to this:

Warning: include(path/to/WP/src/wp-includes/assets/script-loader-react-refresh-entry.php): Failed to open stream: No such file or directory in path/to/WP/src/wp-includes/script-loader.php on line 241

Warning: include(): Failed opening 'path/to/WP/src/wp-includes/assets/script-loader-react-refresh-entry.php' for inclusion (include_path='.;C:\php\pear') in path/to/WP/src/wp-includes/script-loader.php
 on line 241

Warning: include(path/to/WP/src/wp-includes/assets/script-loader-packages.php): Failed to open stream: No such file or directory in path/to/WP/src/wp-includes/script-loader.php on line 276

Warning: include(): Failed opening 'path/to/WP/src/wp-includes/assets/script-loader-packages.php' for inclusion (include_path='.;C:\php\pear') in path/to/WP/src/wp-includes/script-loader.php on line 27
6

Warning: foreach() argument must be of type array|object, bool given in path/to/WP/src/wp-includes/script-loader.php on line 278

This also impacts integration tests for plugins and themes, so as far as I'm concerned, this commit needs to be reverted until a more stable solution has been created.

[gziolo]

Thank you for sharing all the details, @jrf. It means that those workflows for unit tests match the following conditions:

• SCRIPT_DEBUG is defined and set to true - same as when running units on wordpress-develop CI
• WP_RUN_CORE_TESTS is not defined – that would be the difference

We can bring back (remove from git and svn ignore list) all the generated files by npm run build:dev that are missing for the use cases you mentioned.

The alternative would be identifying a constant or a condition that is common for all test workflows, so we don't use dev files there that don't have any impact on unit tests.

[jrf]

@gziolo I can also imagine that this could be solvable by having a check in the test bootstrap.php file for these files and creating a dummy file for each of them if they don't exist ?

More than anything, it concerns me that this change was made without taking integration tests from plugins/themes into account.

[TobiasBg]

Thanks for already reporting the consequences of this change, @jrf. I can confirm that this now raises PHP warnings in my plugin's unit test setup with those include() warnings.

Trac ticket: https://core.trac.wordpress.org/ticket/56615

[gziolo]

I have a patch ​https://github.com/WordPress/wordpress-develop/pull/3340 ready after discussing different options with @hellofromTonya.

[jrf]

Replying to gziolo:

I have a patch ​https://github.com/WordPress/wordpress-develop/pull/3340 ready after discussing different options with @hellofromTonya.

Thanks for working on this @gziolo ! I've left some questions in the PR and tested the patch and can confirm it would fix the issue.

Self-assigning to apply requested changes, test, and prep commit.

Hmm, this PR is failing on my machine when running locally (before and after the force push - rebase on top of trunk):

1) Tests_Dependencies_Scripts::test_wp_add_inline_script_before_after_concat_with_core_dependency
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 console.log("before");
 </script>
 <script type='text/javascript' src='http://example.com' id='test-example-js'></script>
-<script type='text/javascript' src='/wp-includes/js/dist/i18n.min.js' id='wp-i18n-js'></script>
+<script type='text/javascript' src='/wp-includes/js/dist/i18n.js' id='wp-i18n-js'></script>
 <script type='text/javascript' id='wp-i18n-js-after'>
 wp.i18n.setLocaleData( { 'text direction\u0004ltr': [ 'ltr' ] } );
 </script>
-<script type='text/javascript' src='/wp-includes/js/dist/a11y.min.js' id='wp-a11y-js'></script>
+<script type='text/javascript' src='/wp-includes/js/dist/a11y.js' id='wp-a11y-js'></script>
 <script type='text/javascript' src='http://example2.com' id='test-example2-js'></script>
 <script type='text/javascript' id='test-example2-js-after'>
 console.log("after");
 </script>
 '

/var/www/tests/phpunit/includes/abstract-testcase.php:915
/var/www/tests/phpunit/tests/dependencies/scripts.php:761
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

Hmm, this PR is failing on my machine when running locally (before and after the force push - rebase on top of trunk):

I think it might happen when you run npm run build:dev locally which effectively creates the asset file used for dev purposes. In that case, it serves JS files in dev mode with unit tests ... Maybe we should keep the check for core unit tests, too?

Maybe we should keep the check for core unit tests, too?

That's almost certainly the wrong solution direction as anything that depends on checking WP_RUN_CORE_TESTS to _not_ cause failures, _will_ lead to failures in plugin/theme integration test suites.

Hmm, this PR is failing on my machine when running locally (before and after the force push - rebase on top of trunk):

1) Tests_Dependencies_Scripts::test_wp_add_inline_script_before_after_concat_with_core_dependency
Failed asserting that two strings are identical.

Investigating.

I just tried to reproduce this on my machine and the tests seemed to run fine..., but I hadn't run npm in a long time, so I ran the npm install and npm run build:dev commands, then after downgrading node and npm and finally getting the bloody commands to work, I got the same result.

Checked against `trunk and the tests pass, so it is something to do with this PR.

Env which I used to run the tests:

• Windows 10
• PHP 8.2
• PHPUnit 9.5.25 PHAR

All the more interesting that the tests seems to pass in CI, which is yet another reason we need to add a wider variety of test envs in CI and not rely on Docker.

Looking at the test failure, I think this needs to be fixed in the tests by setting better expectations. @hellofromtonya Have you got a fix lined up already or shall I push one ?

As far as I'm concerned this is the patch needed:
{{{diff
--- a/tests/phpunit/tests/dependencies/scripts.php
+++ b/tests/phpunit/tests/dependencies/scripts.php
@@ -720,17 +720,18 @@ JS;

$wp_scripts->base_url = ;
$wp_scripts->do_concat = true;

+ $suffix = wp_scripts_get_suffix();

$ver = get_bloginfo( 'version' );
$suffix = wp_scripts_get_suffix();
$expected = "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&amp;load%5Bchunk_0%5D=jquery-core,jquery-migrate,regenerator-runtime,wp-polyfill,wp-dom-ready,wp-hooks&amp;ver={$ver}'></script>\n";
$expected .= "<script type='text/javascript' id='test-example-js-before'>\nconsole.log(\"before\");\n</script>\n";
$expected .= "<script type='text/javascript' src='​http://example.com' id='test-example-js'></script>\n";

• $expected .= "<script type='text/javascript' src='/wp-includes/js/dist/i18n.min.js' id='wp-i18n-js'></script>\n";

+ $expected .= "<script type='text/javascript' src='/wp-includes/js/dist/i18n{$suffix}.js' id='wp-i18n-js'></script>\n";

$expected .= "<script type='text/javascript' id='wp-i18n-js-after'>\n";
$expected .= "wp.i18n.setLocaleData( { 'text direction\u0004ltr': [ 'ltr' ] } );\n";
$expected .= "</script>\n";

• $expected .= "<script type='text/javascript' src='/wp-includes/js/dist/a11y.min.js' id='wp-a11y-js'></script>\n";

+ $expected .= "<script type='text/javascript' src='/wp-includes/js/dist/a11y{$suffix}.js' id='wp-a11y-js'></script>\n";

$expected .= "<script type='text/javascript' src='​http://example2.com' id='test-example2-js'></script>\n";
$expected .= "<script type='text/javascript' id='test-example2-js-after'>\nconsole.log(\"after\");\n</script>\n";

}}}

I agree with the ​change @jrfnl recommended. However, that change does not resolve the issue for me when running tests locally. Test still fails.

Checked against `trunk and the tests pass, so it is something to do with this PR.

Ran tests locally against current trunk - all passed ✅
Ran tests locally against forked trunk - all passed ✅

The issue is being introduced in this PR.

Good > CI PHPUnit tests are now failing after ​https://github.com/WordPress/wordpress-develop/pull/3340/commits/ed226e8063f6ac566b669d0969e9d44d1d8bf5de

Good > CI PHPUnit tests are now failing after ​ed226e8

Test are failing as only one of the two suggested places for the adding $suffix in the test expectations has been adjusted. If you add the second one, it should be okay AFAICS.

🤦‍♀️ Good catch @jrfnl Duh on my part. Fixed. Rebased to current trunk. Pushed.

Darn, still failing - I feared this may happen as the test are a "special kind of beast" and basing the suffix on SCRIPT_DEBUG, like is done in wp_scripts_get_suffix() was probably the wrong choice (though seemed like a logical one due to the call to wp_scripts_get_suffix() already being there in the test function, even though unused).

In that case... I would suggest changing the suffix determination to be based on ABSPATH.

Reason being:

• We don't want to duplicate logic used in the function under test in the test itself as then we wouldn't be testing anything anymore.
• ABSPATH in a test environment is defined in the wp-test-config.php and will in "normal" (local) test situations end with /src/ and in Docker with /build/ (= CI).

By checking the value of ABSPATH, we should be able to figure out whether we should use .min ( for /build/) or an empty suffix (for /src/).

So something along the lines of (pseudo-code):
{{{diff

• $suffix = wp_scripts_get_suffix();

+ $suffix = str_ends_with(ABSPATH, '/src/') ? : WP_ASSET_SUFFIX_MIN;
}}}

Note, the above is based on an "if I remember correctly" and will need to be verified.

Well lookie lookie @jrfnl's pseudo-code resolves both the CI and local testing issues. Cool. 👏 In review, it makes sense for the reasons she specified.

Tests now pass locally when:

• npm run test:php all tests ✅
• npm run test:php -- --filter test_wp_add_inline_script_before_after_concat_with_core_dependency` ✅
• npm run test:php -- --group dependencies ✅

CI tests still failing. Investigating.

Yes, the wp-tests-config-sample.php file defines ABSPATH to be /src/ ​https://github.com/WordPress/wordpress-develop/blob/trunk/wp-tests-config-sample.php#L4

{{{php
define( 'ABSPATH', dirname( FILE ) . '/src/' );
}}}

That sample files is saved as wp-tests-config.php here ​https://github.com/WordPress/wordpress-develop/blob/trunk/tools/local-env/scripts/install.js#L26-L33.

In that case, we may as well just change the output expectation to use a regex and be done with it.

Or - looking at the code of that particular test now - change the preg_replace() which is applied to the _actual_ output ($print_scripts) to also replace .min.js with .js (and revert the changes in $expected).

To be honest, looking at the test function, I'm not sure what it doesn't properly use expectOutputRegex() in the first place....

[audrasjb]

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

As I'm out sick, I've pinged @azaozz to commit it.

As I'm out sick, I've pinged @azaozz

This PR introduces changes to script-loader and default-constants, it is not a "pure" test PR. Don't see a compelling reason to commit it now, probably better after 6.1 is released.

Also I'm not sure what's the purpose of the new WP_ASSET_SUFFIX_MIN constant and how it is supposed to be used.

ABSPATH in a test environment is defined in the wp-test-config.php and will in "normal" (local) test situations end with /src/ and in Docker with /build/ (= CI).

That's not necessarily true. Think a lot of the wp-tests-config.php files still contain code like:

/* Path to the WordPress codebase you'd like to test. Add a forward slash in the end. */
if ( defined( 'WP_RUN_CORE_TESTS' ) && WP_RUN_CORE_TESTS ) {
	define( 'ABSPATH', dirname( __FILE__ ) . '/build/' );
} else {
	define( 'ABSPATH', dirname( __FILE__ ) . '/src/' );
}

By checking the value of ABSPATH, we should be able to figure out whether we should use .min ( for /build/) or an empty suffix (for /src/).

That would mostly work. Will probably fail in places where the ​WP build repo is cloned. The path there will probably not have /build/ in it.

[hellofromTonya]

​The follow-up PR has changes requested and the tests are failing. Given that the PR touches source code (not just tests or build tools), moving this to 6.3 as 6.2 RC1 is next week and the last scheduled beta already happened.

The PR is close to done. A few tweaks and some follow-up testing it should be ready for committing in 6.3 when done.

Coming back to this, I'm most likely missing something but what is the problem with having both

/assets/script-loader-packages.php
/assets/script-loader-packages.min.php

(committed) in /src and loading the "development" or "production" version according to whether SCRIPT_DEBUG is set? Then can copy only /assets/script-loader-packages.min.php to /build if the non-minified scripts are never going to be useful in production, to save some space.

This is the usual way to define and use JS files in script-loader, why not use it here? That will also make WP_ASSET_SUFFIX_MIN pointless, and generally simplify it a little.

[audrasjb]

Moving to milestone 6.4 as WP 6.3 RC3 has been released.

Is it something that we still need or did it get resolved in a different way?

[marybaum]

We the bug-scrubbers of October will leave yall to this! @hellofromTonya confirms build/test is not bound by release milestones.

[jorbin]

Moving to milestone 6.5 as WP 6.4 RC3 has been released.

[swissspidy]

The two attached PRs here were closed and there's no traction; not clear to me what's still missing but clearly it doesn't look like something that's being worked on in 6.5

Trac ticket:

[desrosj]

Since this has been punted to numbered milestones for 5 release cycles straight, I'm going to punt this to Future Release.