WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#36528 closed enhancement (fixed)

Only run imagemin task inside precommit task if images are added/modified

Reported by: iseulde Owned by: jorbin
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch needs-testing
Focuses: Cc:

Description

Right now it runs all the time.

Attachments (4)

36528.patch (1.4 KB) - added by iseulde 4 years ago.
36528.diff (1.4 KB) - added by jorbin 4 years ago.
36528.2.patch (1.8 KB) - added by iseulde 4 years ago.
36528.3.patch (1.7 KB) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (22)

#1 follow-up: @iseulde
4 years ago

Any objections?

@iseulde
4 years ago

#2 in reply to: ↑ 1 @DrewAPicture
4 years ago

Replying to iseulde:

Any objections?

None here. @jorbin?

#3 @netweb
4 years ago

I've two use cases where this will trip us up:

  1. When core updates grunt-contrib-imagemin without the files being modified they won't benefit from further enhancements of the module.
  1. This is similar to 1, though this time it is due to the nature of NPM's semantic versioning, grunt-contrib-imagemin uses "imagemin": "^4.0.0", hence any further major, minor, or patch level releases in the 4.x branch of Imagemin will be installed by any user who creates a fresh clone/checkout of the develop, or deletes the node_modules folder to run npm install again, or runs npm update, so once again without the files being modified they won't benefit from further enhancements of the module.
Last edited 4 years ago by netweb (previous) (diff)

#4 follow-up: @iseulde
4 years ago

You'll have exactly the same issue with e.g. the postcss task for CSS files. I don't think the precommit task should look at that as you want to make sure your changes are ready for commit or to be uploaded as a patch, and not have all images modified because the module changed. This should be a separate commit and is likely to be the task of someone else, it needs its own ticket etc. At some point someone will run prerelease which will make sure the changes are detected. We could even make sure all tasks with precommit are run if there are no file modifications.

#5 @iseulde
4 years ago

Also, we could look if there's a change in package.json and in that case run all the tasks.

#6 in reply to: ↑ 4 @netweb
4 years ago

Replying to iseulde:

You'll have exactly the same issue with e.g. the postcss task for CSS files.

Indeed, the same issue will happen with PostCSS, and this could be more problematic than the imagemin case as they have more frequent release.

Replying to iseulde:

I don't think the precommit task should look at that as you want to make sure your changes are ready for commit or to be uploaded as a patch, and not have all images modified because the module changed. This should be a separate commit and is likely to be the task of someone else, it needs its own ticket etc. At some point someone will run prerelease which will make sure the changes are detected. We could even make sure all tasks with precommit are run if there are no file modifications.

I disagree with this, after each commit grunt build is ran and these changes are reflected in build.svn.wordpress.org and many people run /trunk live on their sites, the primary repo develop.svn.wordpress.org should be in the most "pristine" condition it can be at all times to reflect the most recent changes for those who run /trunk.

This should *not* be "someone elses" task ran at "some point", this task should be and I believe is one of the responsibilities of a core committers role.

@jorbin
4 years ago

#7 @jorbin
4 years ago

  • Keywords commit added

postcss is just the framework. In and of itself, it does nothing. Not sure how that relates to this conversation. Unless you are referring to auto-prefixer which we only really need to worry about every six weeks or so based on browser releases.

I think we are fine relying on grunt release. As the person most likely to commit an update to grunt-contrib-imagemin, I am also fine running it manually then. Images aren't added often. If someone running trunk is using an image that is a few KB bigger, well I think they are going to be ok.

Patch refreshed as it no longer applied cleanly.

#8 @iseulde
4 years ago

In 37211:

Build/Test Tools: Run image tasks only if there are changes

See #36528.

#9 follow-up: @iseulde
4 years ago

Shall we run these if package.json is changed, or straight run prerelease if it is changed?

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

#10 in reply to: ↑ 9 @jorbin
4 years ago

Replying to iseulde:

Shall we run these if package.json is changed, or straight run prerelease if it is changed?


prerelease seems best to me. Maybe we should also do this if the gruntfile.js changes to be on the safe side? I generally run most of the tasks then anyways just to make sure things are working properly and to see if the build has changed any files.

@iseulde
4 years ago

#11 @iseulde
4 years ago

@jorbin, something like this?

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


4 years ago

#13 @ocean90
4 years ago

  • Owner set to jorbin
  • Status changed from new to reviewing

#14 @jorbin
4 years ago

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

In 37650:

Build/Test Tools: Run prerelease as the precommit task for configuration file changes

Whenever package.json or Gruntfile.js is updated, we should assume that it affects everything and run the full monty of tasks.

Fixes #36528.
Props iseulde.

#15 @azaozz
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This fails pretty bad on Windows. No matter what is changed, no tasks are run and it exits with Done, without errors.. Happens because the line breaks in result.stdout there are \r\n and the strings don't match.

Perhaps we could strip all \r before trying to match, but better to use a bit of Regex? It's made for this :)

Also the code can be more readable. Wrapping arrays plus callback functions in if statements is not. Few inline comments won't hurt either.

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

@azaozz
4 years ago

#16 @azaozz
4 years ago

In 36528.3.patch:

  • Use regex to match in result.stdout.
  • Define the callbacks outside of the if statements.

#17 @azaozz
4 years ago

  • Keywords needs-testing added; commit removed

#18 @azaozz
4 years ago

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

In 37749:

Grunt: when running precommit use regex to check which files have been modified.

Fixes #36528.

Note: See TracTickets for help on using tickets.