Make WordPress Core

Opened 5 years ago

Closed 21 months ago

Last modified 20 months ago

#47749 closed enhancement (fixed)

Build tools: remove all old files when cleaning

Reported by: azaozz's profile azaozz Owned by: azaozz's profile azaozz
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-testing-info
Focuses: Cc:

Description

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.

Attachments (2)

47749.diff (23.3 KB) - added by azaozz 5 years ago.
47749.1.diff (30.9 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (33)

@azaozz
5 years ago

#1 @azaozz
5 years ago

  • Keywords needs-testing added

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.

Last edited 5 years ago by azaozz (previous) (diff)

#2 @dkarfa
5 years ago

  • Keywords dev-feedback added
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.

#3 @dkarfa
5 years ago

My bad, I update the babel runtime

npm install @babel/runtime@latest

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

Last edited 5 years ago by dkarfa (previous) (diff)

#4 @azaozz
5 years ago

  • Keywords dev-feedback removed

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

@azaozz
5 years ago

#5 @azaozz
5 years ago

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.
Last edited 5 years ago by azaozz (previous) (diff)

#6 @desrosj
5 years ago

  • Milestone changed from 5.3 to Future Release

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.

#7 @isabel_brison
4 years ago

  • Keywords needs-refresh added

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?

Last edited 4 years ago by isabel_brison (previous) (diff)

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


22 months ago
#9

  • Keywords has-patch added; needs-refresh removed

Refreshes previous work on Trac 47749.

Props azaozz, dkarfa, desrosj, isabel_brison, SergeyBiryukov.

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

#10 @ironprogrammer
22 months ago

  • Keywords has-testing-info added
  • Milestone changed from Future Release to 6.2

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.

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

@mukesh27 commented on PR #3843:


22 months ago
#11

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

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

@ironprogrammer commented on PR #3843:


22 months ago
#12

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

#13 follow-up: @robinwpdeveloper
22 months ago

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

#14 @razthee007
22 months ago

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

#15 @costdev
21 months ago

  • Keywords commit added; needs-testing removed

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

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


21 months ago

#17 in reply to: ↑ 13 @ironprogrammer
21 months ago

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.

#19 @azaozz
21 months ago

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.

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


21 months ago

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


21 months ago

#22 @peterwilsoncc
21 months ago

  • Keywords changes-requested added; commit removed

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

@azaozz commented on PR #4038:


21 months ago
#23

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.

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


21 months ago

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


21 months ago

@ironprogrammer commented on PR #4038:


21 months ago
#26

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?

@azaozz commented on PR #4038:


21 months ago
#27

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.

#28 @ironprogrammer
21 months ago

  • Keywords changes-requested removed

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

#29 @azaozz
21 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

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.

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


21 months ago

Note: See TracTickets for help on using tickets.