Build tools: remove all old files when cleaning

[azaozz]

Currently running clean doesn't delete all old files. It either cleans only /build or only the inline files in /src. This can get confusing when switching the building location.

The presence of the build files in /src can also cause some problems when doing grunt precommit or building to /build, see #47078.

[azaozz]

In 47749.diff: fix the clean task to clean old files from both /build and /src.

The patch also contains some unrelated WPCS fixes in Gruntfile.js.

[dkarfa]

Loading "Gruntfile.js" tasks...ERROR
>> Error: Cannot find module '@babel/runtime/helpers/interopRequireDefault'
Warning: Task "build" not found. Use --force to continue.

Aborted due to warnings.

[dkarfa]

My bad, I update the babel runtime

npm install @babel/runtime@latest

Then check, working fine with the patch, it worked :).

[azaozz]

@dkarfa Thanks for testing! Yeah, I forget to run npm install sometimes too. Glad it's working as expected.

[azaozz]

Was looking a bit more at what "grunt clean" does and why:

• clean:files cleans everything from /build. Wondering if there's need to separately clean /js and /css there.
• Running Webpack when building the js also creates /src/wp-includes/css/dist. It needs to be "cleaned" when cleaning the js or it may get deleted after it's created.
• After building to /build there's a leftover /src/wp-includes/js directory with some of the js files.

In 47749.1.diff:

• Switch grunt clean:js and grunt clean:css to only clean in /src. Can make that that dependent on passing --dev but not sure it's needed.
• Delete /src/wp-includes/css/dist when running clean:js.
• Run clean:js after building in /build to remove /src/wp-includes/js.

[desrosj]

With 5.3 beta 1 in a few hours, I'm punting this one. However, because it is test and tooling related, it can be moved back and committed during the beta period if someone has time to own.

[isabel_brison]

Tested and the patch no longer applies cleanly.

Also, the WPCS changes make it really hard to read where the actual code changed, so I'm not 100% sure what's happening here. To clarify: the clean task referred to in the description is the one that runs when running grunt build or grunt ​build:dev`, right? So the purpose is to make sure that old files are cleaned from both build/ and src/, whichever directory we're building from?

[SergeyBiryukov]

Related: #53687

Refreshes previous work on Trac 47749.

Props azaozz, dkarfa, desrosj, isabel_brison, SergeyBiryukov.

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

[ironprogrammer]

Testing Instructions

Steps to Test clean Task

1. Build the wordpress-develop dev environment with: npm run build:dev. This generates compiled files in the src/ directory.
2. Confirm compiled files exist by listing files not under source control: git ls-files -o src/wp-admin and git ls-files -o src/wp-includes. Numerous files should be returned.
3. Run npm run grunt clean.
4. Confirm that the compiled files were removed by re-running the commands from Step 2.

Steps to Test build Task

1. Build the wordpress-develop distribution with: npm run build. This generates compiled files in the src/ directory and copies them to build/.
2. Confirm compiled files were cleaned from the src/ directory after build by listing files not under source control: git ls-files -o src/wp-admin and git ls-files -o src/wp-includes. There should be no results.

Expected Results

• ✅ During build, there should be no errors related to compiled files in src/wp-admin or src/wp-includes (see Additional Information).
• ✅ After completing each of the above tests, there should be no leftover files in src/wp-admin or src/wp-includes.

Additional Information

It is possible that during the course of development, a given src/ directory may become populated with files that are excluded from source control. This may include things like patch/merge files (.orig|.rej) or even orphaned directories (src/wp-includes/blocks/some-removed-block/). If a build error occurs due to files like these, they should be manually resolved.

Thanks @ironprogrammer, As per the @azaozz patch 47749.1.diff is there anything missing?

'clean:files' is missing in grunt.registerTask( 'build', function() {

Thanks @ironprogrammer, As per the @azaozz patch 47749.1.diff is there anything missing?

'clean:files' is missing in grunt.registerTask( 'build', function() {

Hi, @mukeshpanchal27 👋🏻

The ​use of `build:dev` affects the src/ directory, so I opted to be conservative and not modify build/, ​which is handled separately. build:dev includes build:js and build:css, which each handle their respective cleaning tasks.

I believe this addresses the reported issue, but maybe @azaozz could confirm 🙏🏻.

[robinwpdeveloper]

Hi @ironprogrammer after following testing instructions I see multisite files are still exists.
Do you think we should cover multisite or just skip?

Thanks for the detail instructions.
Screenshot:
Before - ​https://d.pr/i/aWR7EX
After - ​https://d.pr/i/5qfYhh

[razthee007]

I have followed the testing instructions. Here is the result.

Single site
✅ Clean Task:
Before: wp-includes: ​https://d.pr/i/VAo0e8

wp-admin: ​https://d.pr/i/kVpfxt

After: wp-admin & wp-includes: ​https://d.pr/i/bdVy2w

✅ Build Task:
After Applying Patch: ​https://d.pr/i/fhwMZs

[costdev]

​PR 3843 is testing well for single site. As we are releasing 6.2 Beta 1 today, I'm going to remove needs-testing and mark this ready for commit to help clear the report. As this is a Build/Test tools ticket and can be committed at any time during the cycle, multisite can be handled later in the cycle, or next cycle. 🙂

P.S. I would change this to Task (blessed), but discussion is still ongoing as to when that is appropriate. I'll leave that decision to @azaozz 🙂

Additional props: @mukesh27

[ironprogrammer]

Replying to robinwpdeveloper:

Hi @ironprogrammer after following testing instructions I see multisite files are still exists.
Do you think we should cover multisite or just skip?
...
After - ​https://d.pr/i/5qfYhh

Thanks for the test report, @robinwpdeveloper! The mix of *.orig|*.rej files in the second screenshot are artifacts from patches that didn't apply cleanly, and not a result of issues with the build process itself (see Additional Information in comment:10). The clean/build task isn't specific to single or multisite use.

Based on ​https://github.com/WordPress/wordpress-develop/pull/3843.

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

[azaozz]

Added ​https://github.com/WordPress/wordpress-develop/pull/4038 which is based on ​https://github.com/WordPress/wordpress-develop/pull/3843.

It also removes the CSS and JS built files from /build when building there so the current behavior is not changed.

[peterwilsoncc]

@ironprogrammer has put some change notes on the PR, updating the keywords on this ticket accordingly to keep the milestone reports accurate.

Frankly I'm still "on the fence" between this approach and the approach from ​https://github.com/WordPress/wordpress-develop/pull/3843.

In theory best would be to remove all "built" files from /src before bulk-copying to /build, or the built files should be excluded. In addition to keep the cleanup working the same as now, the js and css built files should be removed from the WORKING_DIR when building. Not sure how critical is that but probably "nice to keep".

Then perhaps the right approach would be to extend clean:files to clean everything in /build (as it does now) plus all build files from /src. This should ensure every new WP build starts with a clean slate.

Thanks, @azaozz! After the ​`clean:files` update, there still remains the untracked file src/wp-includes/js/tinymce/tinymce.js. It sneaks past clean because it is copied to source/ later, during vendor:js.

This file is somewhat of an exception in the vendor:js task. The original patch, 47749.1.diff (line 1408), handled this case by calling clean:js again at the end of build (the same was carried over to ​PR 3843).

Should this be considered here as well?

there still remains the untracked file src/wp-includes/js/tinymce/tinymce.js

Good catch :)
Seems cleaning of this was never fixed after moving all the js files to /src/js (in WP 5.0 I think). As tinymce.js is only needed in /src, seems it should be copied only when building there.

[ironprogrammer]

Thanks for the patch update, @azaozz! LGTM 👍🏻 Removing changes-requested.

Test Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/4038

Environment

• Hardware: MacBook Pro Apple M1 Pro
• OS: macOS 12.6.3
• Server: nginx/1.23.3
• PHP: 7.4.33
• WordPress: 6.2-beta1-55292-src

Actual Results

clean Task

• ✅ All built files were removed from src/.
• ✅ All built files were removed from build/.

build Task

• ✅ All built files were removed from src/.

build:dev Task -- tested since ​06dcfb7

• ✅ Unminified tinymce.js remains in src/.

[azaozz]

In 55484:

Build/Test Tools: Remove all previously built files when running clean:files.

Cleans old JS, CSS and Webpack files from /src so they are not automatically copied to /build when running grunt build. Fixes errors that may be caused by copying outdated files and/or directories to /build.

Props: desrosj, isabel_brison, SergeyBiryukov, ironprogrammer, mukesh27, robinwpdeveloper, razthee007, costdev, peterwilsoncc, azaozz.

Fixes: #47749.

Closed in favor of ​https://github.com/WordPress/wordpress-develop/pull/4038.