Make WordPress Core

Opened 6 months ago

Closed 5 months ago

Last modified 5 months ago

#60520 closed task (blessed) (fixed)

Update `precommit:emoji` following GitHub's deprecation of SVN support.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: desrosj's profile desrosj
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch needs-testing
Focuses: Cc:

Description

In a move few people noticed, GitHub have deprecated their support of SVN.

As the emoji:precommit script makes use of SVN to obtain a list of files in the twemoji repository, this has broken the update of formatting.php by the precommit script.

At present, this leaves committers to manually update the formatting file via alternative methods. Let's explore something that will work for all.

A workaround was created for PR#5804 but discussing this with @desrosj we decided to action the build step in a follow up ticket in the hope of avoiding introducing the GitHub CLI as a requirement.

Follow up to #57600

Change History (12)

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


6 months ago
#1

  • Keywords has-patch added

All build script related changes form #5804 have been copied over.

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

#2 @desrosj
6 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 6.5
  • Type changed from defect (bug) to task (blessed)

After [57626], I've created a PR with only the build related changes to work on this.

@kraftbj commented on PR #6107:


5 months ago
#3

Tested. Only thing I ran into was permission needing updating on the shell script. Submitted a PR to add that via https://github.com/desrosj/wordpress-develop/pull/153

@desrosj commented on PR #6107:


5 months ago
#4

Thanks @kraftbj! I've merged that.

To bring a brief summary here, here are a few things I don't love about the current implementation (though not strongly opposed to):

  • I don't love adding gh as a requirement for the script.
  • I would prefer that the necessary script be included in Grunt.js instead of a separate directory for shell scripts (where this will be the only one).

@kraftbj commented on PR #6107:


5 months ago
#5

I don't love adding gh either, but given that we _do_ use GitHub as a partner service extensively already, it seems okay for now to get this out the door.

I tried to make it all part of the Grunt JS file in https://github.com/desrosj/wordpress-develop/pull/154 -- can you give that a spin?

@desrosj commented on PR #6107:


5 months ago
#7

Thanks all for keeping this going! I think we're at a good spot. There's just one outstanding question about what seems like an unintentional deleted line.

@kraftbj commented on PR #6107:


5 months ago
#8

I concur. If the deletion was intentional (I'm not sure why it would be), it should be noted in the commit to explain the connection. But, with that line restored, LGTM.

@desrosj commented on PR #6107:


5 months ago
#9

It was me that removed that line, wasn't it? Just realized that looking through this again. 🙃

Since I can't recall why I included that, or see a reason how it's related, I've gone and removed it.

I also added a new step to the Actions workflow that tests our build process to run the precommit:emoji script to confirm all expected changes are included. GitHub is not syncing pushes right now for some reason. But once I confirm it correctly flags missing changes, I'll get this merged.

@peterwilsoncc commented on PR #6107:


5 months ago
#10

I also added a new step to the Actions workflow that tests our build process to run the precommit:emoji script to confirm all expected changes are included. GitHub is not syncing pushes right now for some reason. But once I confirm it correctly flags missing changes, I'll get this merged.

As the script currently reads the files from the main branch, this may need to use something version specific so it doesn't trigger build failures due to modified files as the folder is updated upstream.

@desrosj commented on PR #6107:


5 months ago
#11

As the script currently reads the files from the main branch, this may need to use something version specific so it doesn't trigger build failures due to modified files as the folder is updated upstream.

I've tested using a tag reference instead of branch, and it seems to work alright. Talked through this a bit in Slack with @peterwilsoncc and it seems better to tackle this separately. Since it adds an additional spot to update when updating the library, it would be nice to automate it a bit.

#12 @desrosj
5 months ago

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

In 57758:

Build/Test Tools: Fix the precommit:emoji script.

GitHub recently sunset support for Subversion, causing the precommit:emoji Grunt script to break. Since there’s no direct replacement for svn ls in Git, this has been replaced with a query through the GitHub CLI.

This also adds a step in the workflow that tests the build process to run the precommit:emoji script to ensure no changes to built files are missed when updating the Twemoji library in the future.

Follow up to [57626].

Props kraftbj, peterwilsoncc, swissspidy.
Fixes #60520. See #57600.

Note: See TracTickets for help on using tickets.