WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 3 months ago

#53687 reopened defect (bug)

`grunt verify:old-files` incorrectly flagging certain files

Reported by: desrosj Owned by:
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.7
Component: Build/Test Tools Keywords: needs-testing needs-patch
Focuses: Cc:

Description

In [50441], a PHPUnit test that verified built files were not included in the $_old_files list was moved into a Grunt task, verify:old-files.

For some reason, this is incorrectly flagging a few files that were removed and should be included in this list for WordPress 5.8:

  • wp-includes/css/dist/editor/editor-styles.min.css
  • wp-includes/css/dist/editor/editor-styles-rtl.css
  • wp-includes/css/dist/editor/editor-styles-rtl.min.css
  • wp-includes/css/dist/editor/editor-styles.css

A build error is only encountered when running build, build:dev does not result in an error.

It seems that the -rtl and .min parts of the file are the culprit because wp-includes/css/dist/editor/editor-styles.css is not resulting in the task failing.

I don't necessarily think that this is a blocker for WordPress 5.8. The $_old_files list is run over for every update. So these files can be added in 5.8.1 and they will be removed appropriately.

Attachments (1)

53687.diff (612 bytes) - added by desrosj 3 months ago.

Download all attachments as: .zip

Change History (10)

@desrosj
3 months ago

#1 @desrosj
3 months ago

  • Keywords needs-patch added

For more context, after applying 53687.diff, running npm run build results in the following:

Running "verify:old-files" task
Warning: build/wp-includes/css/dist/editor/editor-styles.min.css should not be present in the $_old_files array. Use --force to continue.

#2 @johnbillion
3 months ago

Chatting with Jon about this, the script is behaving as expected as the failure only appears when these files already exist in src and are copied over with the build. If these files have been removed from version control and are now built, I think there should be a cleanup so they're removed from src.

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


3 months ago

#5 @johnbillion
3 months ago

  • Milestone 5.8.1 deleted
  • Resolution set to invalid
  • Status changed from new to closed

Closing this off as the problem is not with the verify:old-files task. The warning it raised when the files were added to $_old_files was correct because the files still existed in the local copy.

#6 @azaozz
3 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Just ran into this too.

the problem is not with the verify:old-files task

Yes, but the results is that npm run build a.k.a. grunt build fails/is aborted.

failure only appears when these files already exist in src and are copied over with the build. If these files have been removed from version control and are now built, I think there should be a cleanup so they're removed from src.

This is somewhat similar to #47078. A cleanup of the old built files from both /src and /build sounds like the better solution. Also see #47749.

However cleaning of all built files will not work at the moment as the above four files (and many others) are not being "cleaned" from /src anyway. Running grunt clean --dev leaves quite a few "built" files behind. Then grunt copy:files copies a mess of "original" and "built" files. Then most of the copied built files are overwritten when building the js and css or by webpack.

For this ticket is seems it would be enough to exclude the 4 offending files from copying to /build, for now. A better way to fix this and similar cases in the future is to fix grunt clean --dev and run it together with grunt clean before every build.

Going to reopen this as it prevents building to /build after building to /src.

Last edited 3 months ago by azaozz (previous) (diff)

#7 @azaozz
3 months ago

  • Milestone set to 5.9

#8 follow-up: @desrosj
3 months ago

@azaozz Would the solution proposed on #53719 solve this?

#9 in reply to: ↑ 8 @azaozz
3 months ago

Replying to desrosj:

Yes but that cleaning of /src will have to happen before building to /build. To fix this ticket it would need the solution proposed on #53719 plus running grunt clean on both /src and /build.

Note: See TracTickets for help on using tickets.