Proof of concept: Remove the requirement to check out the gutenberg repository and running the full build script#11019
Conversation
desrosj
force-pushed
the
try/remove-gutenberg-git-checkout-and-build
branch
from
edf4224 to
2e3f506
Compare
desrosj
changed the title
gutenberg repository and running two build scriptsgutenberg repository and running the full build script
|
There is one remaining issue to figure out: how to map the I've removed the chunk of code responsible for mapping those files to their respective locations within |
|
It seems that the icons are not currently included in the Gutenberg plugin, only in @mcsf I see you introduced this in 5b7a3ad. Could you share a bit more context as to why these are not yet included in the Gutenberg plugin? I don't see them in the latest plugin version, but I may be missing them. |
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
this is a step up @desrosj and should avoid a lot of the delay when checking out the repo. 🙇♂️
does the zip contain the proper built versions of the artifacts or does it contain source copies? that is, are there any build steps that are occurring on the Gutenberg side that occur before that ZIP is available?
it would definitely be helpful to know in the docs, at least in download-gutenberg.js because I think it’s really not clear if we are downloading the Gutenberg source or some output. if output, I would love to know on the wordpress-develop side what our expectations are of that artifact and why .layers[0].digest is special.
Perhaps there is a command we could include which comparably would generate the right files were we given the Gutenberg source.
is it right that this still leaves the icon disparity in place?
| fs.readFileSync( packageJsonPath, 'utf8' ) | ||
| ); | ||
| sha = packageJson.gutenberg?.sha; | ||
| ghcrRepo = packageJson.gutenberg?.ghcrRepo; |
There was a problem hiding this comment.
are we short on bytes? the gh part is obvious to me as a reader, but I cannot figure out what cr means, and since this only appears to be documented inside this file, I’m not sure it would be clear from the package.json file, especially since it looks like a standard GitHub repo name.
perhaps something githubRepo would suffice? it doesn’t seem like the package.json value has anything specific to do with the Github Container Registry
There was a problem hiding this comment.
I am new to this as well, so I found that GHCR stands for the GitHub Container Registry
There was a problem hiding this comment.
I've added a note within the inlind docblock to explain this at the first occurrence of ghcrRepo. Does that address your concerns here @dmsnell?
I also changed the name of the package to gutenberg-wp-develop-build, making the full reference WordPress/gutenberg/gutenberg-wp-develop-build. It's more specific to the purpose, and hopefully won't lead to any confusion.
Within the GHCR, packages are published to a specific repository. While they are listed on a separate screen, you need to use the Org/Repository/package path to reference them.
Oof, I'm so glad you pinged me! Fix at WordPress/gutenberg#75866 |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| throw new Error( `Failed to download blob: ${ response.status } ${ response.statusText }` ); | ||
| } | ||
| const buffer = await response.arrayBuffer(); | ||
| fs.writeFileSync( zipPath, Buffer.from( buffer ) ); |
There was a problem hiding this comment.
at this point it’s probably possible to remove all of the _Sync versions and stream the output directly into the file.
not essential, but it does feel like we’re possibly going out of our way to do more work to avoid a simpler default.
await fs.writeFile( zipPath, response.body );There was a problem hiding this comment.
I'm not deeply knowledgeable here, but Claude told me that this wouldn't work as you've written "since fs.promises.writeFile doesn't accept a ReadableStream — stream.pipeline is the correct API for this." Does this seem right now?
|
I still need to go through and polish this, but the PR for the Gutenberg repository is ready. That should be merged first anyway, and these changes can't be committed until after the first hash update following that PR being merged. So I'm hoping to clean this up on Monday. |
My initial rough measurements were showing that the hashing takes around 150-200ms with the downloaded pre-built plugin files, and ~2.5 seconds with the full repository checkout. |
| response.body, | ||
| zlib.createGunzip(), | ||
| Writable.toWeb( tar.stdin ), | ||
| ); |
There was a problem hiding this comment.
ah darn, sorry if I leaned too hard into this and we still have to call out to the shell.
This switches to downloading a prebuilt zip file uploaded to the GitHub Container Registry for a given commit instead. This eliminates the need to run any Gutenberg build scripts within `wordpress-develop`.
A REF is typically a human-readable path to a branch where as a SHA is a hash representing an individual commit. Since the latter is what's being used here, this aims to avoid confusion.
It's redundant to include `gutenberg` in the file names.
This has better native support in Node.
This removes some unused code and variables, updates some comments to be more accurate, and ensures all one line inline comments end with a period.
desrosj
force-pushed
the
try/remove-gutenberg-git-checkout-and-build
branch
from
50e571e to
50c6285
Compare
|
I talked through this PR with @dmsnell today, and we arrived at the decision that it was best to leave all aspects of using the While it's possible someone would could be using the We also decided that for the time being, there's no need to hash the entire I've gone and rebased all of the changes onto the latest from |
There are a few pre-existing grunt tasks that are performing identical tasks to some of the code in `copy.js`. Mainly: - `grunt-contrib-copy` copies files with the ability to perform custom transformations. - `grunt-contrib-replace` replaces strings and pattern matches, which is primarily used to remove `sourceMappingURL` from JavaScript files.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I also realized that a good portion of the custom code in the I've made adjustments so that as much as possible (where other complex modifications are not being made), the Gutenberg-related files are now copied by |
desrosj
requested review from
aaronjorbin,
ellatrix,
peterwilsoncc and
youknowriad
and removed request for
youknowriad
| // Fix boot module asset file path for Core's different directory structure. | ||
| return content.replace( | ||
| /__DIR__\s*\.\s*['"]\/..\/\..\/modules\/boot\/index\.min\.asset\.php['"]/g, | ||
| 'ABSPATH . WPINC . \'/js/dist/script-modules/boot/index.min.asset.php\'' |
There was a problem hiding this comment.
| 'ABSPATH . WPINC . \'/js/dist/script-modules/boot/index.min.asset.php\'' | |
| "ABSPATH . WPINC . '/js/dist/script-modules/boot/index.min.asset.php'" |
| process: function( content ) { | ||
| // Fix boot module asset file path for Core's different directory structure. | ||
| return content.replace( | ||
| /__DIR__\s*\.\s*['"]\/..\/\..\/modules\/boot\/index\.min\.asset\.php['"]/g, |
There was a problem hiding this comment.
| /__DIR__\s*\.\s*['"]\/..\/\..\/modules\/boot\/index\.min\.asset\.php['"]/g, | |
| /__DIR__\s*\.\s*(['"])\/..\/\..\/modules\/boot\/index\.min\.asset\.php\1/g, |
minor nit, and unlikely to cause any trouble, but when we match the opening quote character we can make sure we match the terminating quote of the same kind.
problems should already be evident within the PHP if we have something like this, and an over-match here with this pattern would/should do no harm, but it’s not terrible to match assertively here on matching quotes.
// this matches ['"]...['"] but not ['"]...\1
require __DIR__ . '/../../modules/boot/index.min.asset.php";| '"$schema": "../schemas/json/theme.json"', | ||
| '"$schema": "https://schemas.wp.org/trunk/theme.json"' | ||
| ); | ||
| } |
There was a problem hiding this comment.
this does in a string things that are likely to mis-parse or be mislead. again, there should be extra layers of protection here, however, it is not likely that much more difficult to do this in a more robust way.
process: function( content, srcpath ) {
if ( path.basename( srcpath ) !== 'theme.json' ) {
return content;
}
return JSON.stringify(
JSON.parse( content ),
( key, value ) =>
( '$schema' === key && '../schemas/json/theme.json' === value )
? 'https://schemas.wp.org/trunk/theme.json'
: value,
2
);
}this is marginally larger but contains none of the issues with treating JSON as a string. it’s not a big deal here, so not a blocker in any way, but it would be nice if we could at least follow-up after fixing things to return to these low-level build functions and make sure they don’t contain foot-guns for handling well-specified data.
| }, | ||
| { | ||
| src: 'gutenberg/lib/theme-i18n.json', | ||
| dest: WORKING_DIR + 'wp-includes/theme-i18n.json', |
There was a problem hiding this comment.
while it looks like theme-i18n.json contains no $schema property, is there a reason to explicitly avoid changing the relative path to an absolute one? it seems somewhat odd given that this list of files includes two files and the process function only operates on one of them.
not really a block, but something perhaps to earmark.
| '_x( $1, $2 )' | ||
| ); | ||
| } | ||
| return content; |
There was a problem hiding this comment.
here again we’re apparently adding automation which nobody is likely to be maintaining familiarity with, doing something that looks sound today when all of the files are the way we expect them, and then one day it will all break and nobody will know what the goal here is. we’re also doing naive string parsing of JSON files again.
it seems like Gutenberg’s packages/icons/lib/generate-manifest-php.cjs is building the manifest.php from the JSON source anyway, so I wonder if we could parameterize that method with the intended text domain and simply regenerate this file.
should the shape of the manifest change, I have no idea how to expect someone to know what the original goal of this RegExp was supposed to be or do.
this is definitely a nice-to-have, particularly since the generator file should be available whenever we have the manifest.php file, but also potentially something to address in a follow-up.
I would think we could directly call the generator, like return iconPhpManifestGenerator.generateManifestPHP( null ) as long as we add the text domain parameters where appropriate
| // Run main function. | ||
| main().catch( ( error ) => { | ||
| console.error( '❌ Unexpected error:', error ); | ||
| process.exit( 1 ); |
There was a problem hiding this comment.
I started working through this file locally because I was finding it hard to review in GitHub, and it was hard to review on its own. lots of unclear logic, clearly fragile code, duplication, misleading names and comments, and hard-coded ad-hoc details 🤦♂️
would be nice to think about replacing this file entirely, but after 7.0.
thank you for all your hard work nudging through this.
one thing I did find is that this has hard-coded logic for the vips library, which is really surprising to me, buried in the middle of this large process. I know there was some concern about VIPS loading 3.5 MB of compressed JS (which expands to 10 MB of JS in the browser). if people do end up modifying what we do with client-side image manipulation I find it hard to imagine they would find this code here to update it accordingly.
|
@desrosj in 3050cd1 I replaced the string/RegExp-based PHP parser with as I’ve reviewed this today I keep finding places we’re doing the lots of repetitive work which doesn’t even need to be done, but it’s scattered through the files. for example, a lot of these PHP files are generated in Gutenberg from JSON, and then parsed as string in here in so I apologize, but this last statement isn’t fully actionable; just a little venting and one contributing factor to why it’s taken me so long to review this or the original change that introduced the code modified here. |
4 participants
This PR attempts to remove the requirement to checkout the WordPress/gutenberg Git repository locally to run the
gutenbergrepository build script as a part of thewordpress-developone.Instead a copy of the built plugin is downloaded from the GitHub Container Registry for the respective pinned hash value (see WordPress/gutenberg#75844 for that part of this approach). This is now the default behavior.
There is also overlap in preexisting Grunt tasks and what the related scripts currently do. This moves that behavior into Grunt to eliminate unnecessary custom code (mainly copy and replace commands).
Other changes in this PR:
reftoshato avoid confusion. Ref is typically a human-readable branch path. A commit hash or SHA value is the full length, unique hash for a commit.grunttasks have been renamed to followgutenberg:commandinstead ofgutenberg-commandpattern.gutenberghas been removed from all file names withintools/gutenbergsince it's redundant.Trac ticket: Core-64393.
Use of AI Tools
I used Claude Code pretty heavily throughout the work on this pull request.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.