#47749 closed enhancement (fixed)
Build tools: remove all old files when cleaning
Reported by: | azaozz | Owned by: | 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)
Change History (33)
#2
@
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
@
5 years ago
My bad, I update the babel runtime
npm install @babel/runtime@latest
Then check, working fine with the patch, it worked :).
#4
@
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.
#5
@
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
andgrunt 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 runningclean:js
. - Run
clean:js
after building in/build
to remove/src/wp-includes/js
.
#6
@
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
@
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?
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
@
22 months ago
- Keywords has-testing-info added
- Milestone changed from Future Release to 6.2
Testing Instructions
Steps to Test clean
Task
- Build the
wordpress-develop
dev environment with:npm run build:dev
. This generates compiled files in thesrc/
directory. - Confirm compiled files exist by listing files not under source control:
git ls-files -o src/wp-admin
andgit ls-files -o src/wp-includes
. Numerous files should be returned. - Run
npm run grunt clean
. - Confirm that the compiled files were removed by re-running the commands from Step 2.
Steps to Test build
Task
- Build the
wordpress-develop
distribution with:npm run build
. This generates compiled files in thesrc/
directory and copies them tobuild/
. - 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
andgit ls-files -o src/wp-includes
. There should be no results.
Expected Results
- ✅ During
build
, there should be no errors related to compiled files insrc/wp-admin
orsrc/wp-includes
(see Additional Information). - ✅ After completing each of the above tests, there should be no leftover files in
src/wp-admin
orsrc/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.
@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 ingrunt.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:
↓ 17
@
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
@
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
@
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
@
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.
This ticket was mentioned in PR #4038 on WordPress/wordpress-develop by @azaozz.
21 months ago
#18
#19
@
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
@
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.
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?
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
@
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 insrc/
.
#29
@
21 months ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 55484:
This ticket was mentioned in Slack in #core by sergey. View the logs.
21 months ago
@ironprogrammer commented on PR #3843:
20 months ago
#31
Closed in favor of https://github.com/WordPress/wordpress-develop/pull/4038.
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.