Make WordPress Core

Opened 3 years ago

Closed 17 months ago

#53687 closed defect (bug) (fixed)

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

Reported by: desrosj's profile desrosj Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.7
Component: Build/Test Tools Keywords: needs-testing needs-patch needs-refresh
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 years ago.

Download all attachments as: .zip

Change History (29)

@desrosj
3 years ago

#1 @desrosj
3 years 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 years 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 years ago

#5 @johnbillion
3 years 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 years 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 either 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.

Version 0, edited 3 years ago by azaozz (next)

#7 @azaozz
3 years ago

  • Milestone set to 5.9

#8 follow-up: @desrosj
3 years ago

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

#9 in reply to: ↑ 8 @azaozz
3 years 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.

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


3 years ago

#11 @chaion07
3 years ago

Hi @desrosj! Thank you for reporting this. Leaving it on the report for 5.9 and also be happy to push it to future release. Since the build tools don't affect the release code, and also someone can pick it up even during the RC period, keeping my fingers crossed. Cheers!

#12 @hellofromTonya
3 years ago

  • Milestone changed from 5.9 to 6.0

With 5.9 Beta 1 release happening in a few hours, moving this ticket to 6.0.

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


3 years ago

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


3 years ago

#15 @costdev
3 years ago

  • Keywords needs-refresh added
  • Milestone changed from 6.0 to Future Release

Per the discussion in the bug scrub, I'm moving this ticket to Future Release. The patch also needs a refresh, so adding the needs-refresh keyword too.

Last edited 3 years ago by costdev (previous) (diff)

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


2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

#20 @hellofromTonya
2 years ago

  • Milestone changed from Future Release to 6.1
  • Owner set to hellofromTonya
  • Status changed from reopened to assigned

Ran into this issue today during Dry Run when running npm run grunt prerelease. An older file, i.e. src/wp-includes/blocks/heading/editor.css, was created from an older npm run build:dev. That files was removed in WP 5.9. As a result, the verify:old-files grunt task failed as the file was added into the build folder.

As @azaozz suggests, cleaning src before running build would have resolved the issue and the time lost during Dry Run.

Moving this ticket into 6.1 for resolution before 6.1 Dry Run cycle.

#21 @desrosj
2 years ago

  • Milestone changed from 6.1 to 6.2

This still needs a patch. Punting with 6.1 coming to a close. If a patch is ready and tested before then and a committer is confident in merging, it can be moved back.

#22 @danielbachhuber
2 years ago

I ran into this too:

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

I like to use npm run build when I don't need the watch process running.

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


21 months ago

#24 @hellofromTonya
21 months ago

  • Milestone changed from 6.2 to 6.3

This still needs a patch. With RC1 next week, punting to 6.3 (which alpha opens next week). However, it can be committed at any time when a patch is ready and a committer is confident in committing it.

#25 follow-up: @SergeyBiryukov
21 months ago

Does [55484] / #47749 resolve this?

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


17 months ago

#27 @oglekler
17 months ago

Hi @desrosj and @azaozz can you please take a look if this issue is solved / can be solved by tickets mentioned in the previous comment and if not, possibly page can be refreshed. Let's solve it in 6.3 🙂 Thank you 🙏

#28 in reply to: ↑ 25 @azaozz
17 months ago

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

Replying to SergeyBiryukov:

Does [55484] / #47749 resolve this?

Yep, think it does, sorry for missing to close it.

Lets close as fixed with [55484]. Please feel free to reopen if somebody runs into this again.

Note: See TracTickets for help on using tickets.