Make WordPress Core

Opened 22 months ago

Last modified 2 weeks ago

#56615 reopened defect (bug)

Running `build` scripts on Windows machines produces different results

Reported by: desrosj's profile desrosj Owned by: gziolo's profile gziolo
Milestone: Future Release Priority: normal
Severity: normal Version: 6.1
Component: Build/Test Tools Keywords: needs-testing needs-patch
Focuses: Cc:

Description

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

Change History (55)

This ticket was mentioned in Slack in #core by desrosj. View the logs.


22 months ago

#2 @desrosj
22 months ago

  • Version changed from 6.0 to trunk

#3 @desrosj
22 months ago

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.

#4 @desrosj
22 months ago

Related: #55616.

This ticket was mentioned in PR #3309 on WordPress/wordpress-develop by gziolo.


22 months ago
#5

  • Keywords has-patch added

jsnajdr commented on PR #3309:


22 months ago
#6

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

gziolo commented on PR #3309:


22 months ago
#7

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 commented on PR #3309:


22 months ago
#8

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.

#9 @gziolo
22 months ago

  • Owner set to gziolo
  • Status changed from new to assigned

#10 @gziolo
22 months ago

  • Keywords needs-testing added

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

ockham commented on PR #3309:


22 months ago
#11

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.

#13 @gziolo
22 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#15 follow-up: @jrf
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#16 in reply to: ↑ 15 @gziolo
22 months ago

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.

#17 @jrf
22 months ago

@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.

Last edited 22 months ago by jrf (previous) (diff)

#18 @TobiasBg
22 months ago

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.

This ticket was mentioned in Slack in #core-committers by gziolo. View the logs.


22 months ago

#21 follow-up: @gziolo
22 months ago

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

#22 in reply to: ↑ 21 @jrf
22 months ago

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.

hellofromtonya commented on PR #3340:


22 months ago
#23

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

hellofromtonya commented on PR #3340:


22 months ago
#24

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

gziolo commented on PR #3340:


22 months ago
#25

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?

jrfnl commented on PR #3340:


22 months ago
#26

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.

jrfnl commented on PR #3340:


22 months ago
#27

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 ?

jrfnl commented on PR #3340:


22 months ago
#28

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";

}}}

hellofromtonya commented on PR #3340:


22 months ago
#29

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

hellofromtonya commented on PR #3340:


22 months ago
#30

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.

jrfnl commented on PR #3340:


22 months ago
#32

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.

hellofromtonya commented on PR #3340:


22 months ago
#33

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

jrfnl commented on PR #3340:


22 months ago
#34

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;
}}}

jrfnl commented on PR #3340:


22 months ago
#35

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

hellofromtonya commented on PR #3340:


22 months ago
#36

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

hellofromtonya commented on PR #3340:


22 months ago
#37

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.

jrfnl commented on PR #3340:


22 months ago
#38

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....

#39 @audrasjb
22 months ago

  • Milestone changed from 6.1 to 6.2

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.

@hellofromTonya commented on PR #3340:


21 months ago
#40

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

@azaozz commented on PR #3340:


21 months ago
#41

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.

@azaozz commented on PR #3340:


21 months ago
#42

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.

This ticket was mentioned in Slack in #core-committers by azaozz. View the logs.


17 months ago

This ticket was mentioned in Slack in #core by costdev. View the logs.


17 months ago

#45 @hellofromTonya
17 months ago

  • Keywords changes-requested added
  • Milestone changed from 6.2 to 6.3

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.

@azaozz commented on PR #3340:


17 months ago
#46

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.

#47 @audrasjb
12 months ago

  • Milestone changed from 6.3 to 6.4

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

This ticket was mentioned in Slack in #core-test by sergey. View the logs.


12 months ago

@gziolo commented on PR #3340:


11 months ago
#49

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

This ticket was mentioned in Slack in #core by marybaum. View the logs.


10 months ago

#51 @marybaum
10 months ago

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

#52 @jorbin
9 months ago

  • Milestone changed from 6.4 to 6.5

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

#53 @swissspidy
5 months ago

  • Keywords needs-patch added; has-patch changes-requested removed
  • Milestone changed from 6.5 to 6.6

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

This ticket was mentioned in PR #6960 on WordPress/wordpress-develop by @desrosj.


2 weeks ago
#54

  • Keywords has-patch added; needs-patch removed

Trac ticket:

#55 @desrosj
2 weeks ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 6.6 to Future Release

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

Note: See TracTickets for help on using tickets.