WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 13 days ago

#44492 new enhancement

Add new build:dev task which symlinks all files that can be symlinked

Reported by: omarreiss Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-testing
Focuses: Cc:

Description (last modified by SergeyBiryukov)

This ticket introduces a new build task, meant specifically for development: grunt build:dev.

The main difference with a regular build is that files are symlinked when possible instead of copied. This saves a lot of build time and means you don't have to rebuild anymore for most PHP changes. Since PHP files in build/ are now symlinked to src/, changes are picked up directly.

The new grunt build:dev task first checks if it can create a symlink on the system it runs on. If this is not the case, it will fallback to copying. grunt watch will also trigger build:dev instead of build now since it is only used in dev environments anyway.

Files that are exempted from symlinking are the JS and CSS files, the themes, embed.php, formatting.php and version.php. This is because these are all transformed in some way when built. When someone adds a new file, they would still have to build to have it included.

This ticket was inspired by issues that were raised after #43055 got committed, mostly in the #core-committers channel on Slack. The main issues I've seen are the following:

  • Some people have lots of plugins in their wp-content folder. Building means copying all the plugin files over to /build. For some this crashes.
  • Developing with grunt watch can give issues on some development environments that run in VirtualBox (like VVV), where changes aren't being picked up. Having to rebuild manually after each change is a hassle.
  • XDebug breakpoints set in src/ wouldn't be picked up.

I believe these issues should mostly be mitigated or resolved through this ticket. I wasn't able to test so myself so I could use some help reviewing and confirming this is indeed a big improvement.

Attachments (4)

build-with-symlinks.patch (6.8 KB) - added by omarreiss 3 weeks ago.
build-with-symlinks2.patch (6.8 KB) - added by omarreiss 3 weeks ago.
Rebased
build-with-symlinks3.patch (14.1 KB) - added by omarreiss 2 weeks ago.
Makes clean task a little bit less eager, cleans the symlinks before cleaning the files, shows better errors when symlinking fails.
build-with-symlinks4.patch (9.0 KB) - added by omarreiss 13 days ago.
Makes sure wp-load.php isn't symlinked because the file needs to be physically in the build folder.

Download all attachments as: .zip

Change History (20)

This ticket was mentioned in Slack in #core-committers by omarreiss. View the logs.


3 weeks ago

#2 @SergeyBiryukov
3 weeks ago

  • Milestone changed from Awaiting Review to 5.0

#3 @SergeyBiryukov
3 weeks ago

  • Component changed from General to Build/Test Tools

#4 @netweb
3 weeks ago

Looking at https://www.npmjs.com/package/grunt-contrib-symlink#usage-tips-on-microsoft-windows:

Usage tips on Microsoft Windows

Make sure your command prompt has administrative privileges, otherwise the task will not work.

This is more than likely going to be an issue for the majority of Windows users, I expect it will be particularly troublesome if users are using the PowerShell terminal over cmd.exe also.

#5 follow-up: @flixos90
3 weeks ago

Tested this in VVV while working on #44466, and it works great for me!

Although we typically tend to commit things when we are fairly sure they are solid, for the sake of getting actual usage and testing (and generally improving the workflow right away), I'd suggest to commit iteratively here. What is currently available could go in right now, and if/as issues come up, we can solve them gradually.

Last edited 3 weeks ago by flixos90 (previous) (diff)

#6 in reply to: ↑ 5 @netweb
3 weeks ago

Replying to flixos90:

Although we typically tend to commit things when we are fairly sure they are solid, for the sake of getting actual usage and testing (and generally improving the workflow right away), I'd suggest to commit iteratively here. What is currently available could go in right now, and if/as issues come up, we can solve them gradually.

Let's get some Windows testing reported before committing this, @azaozz @SergeyBiryukov if you could please :)

#7 follow-up: @omarreiss
3 weeks ago

This is more than likely going to be an issue for the majority of Windows users, I expect it will be particularly troublesome if users are using the PowerShell terminal over cmd.exe also.

@netweb I've made it so that it gracefully falls back to copying files when that happens. When symlinking causes errors, I also output the following warning: Error: failed to create a symlink on your system. Will use copy instead.

When running grunt with --verbose it will also output the exact error message.

I think this should be relatively safe to commit, also since the build:dev task is an addition to what is already there and doesn't really alter anything else.

#8 in reply to: ↑ 7 @netweb
3 weeks ago

Replying to omarreiss:

This is more than likely going to be an issue for the majority of Windows users, I expect it will be particularly troublesome if users are using the PowerShell terminal over cmd.exe also.

@netweb I've made it so that it gracefully falls back to copying files when that happens. When symlinking causes errors, I also output the following warning: Error: failed to create a symlink on your system. Will use copy instead.

Great 💯

When running grunt with --verbose it will also output the exact error message.

I think this should be relatively safe to commit, also since the build:dev task is an addition to what is already there and doesn't really alter anything else.

I'd still like to see some feedback from core committers that use Windows before this is committed please.

#9 follow-up: @flixos90
3 weeks ago

I think this should be relatively safe to commit, also since the build:dev task is an addition to what is already there and doesn't really alter anything else.

I agree with this. Let's make contributors' lives better as soon as we can, and iterate to the point where we are able to accomplish that for everyone.

@netweb I definitely agree that we need to dive into the issue and make it work on Windows too. But committing what we have at the moment will not make it harder for you to do what you were able to do before. Therefore I suggest doing a first commit now, but not closing the ticket with it.

#10 in reply to: ↑ 9 @netweb
3 weeks ago

Replying to flixos90:

I think this should be relatively safe to commit, also since the build:dev task is an addition to what is already there and doesn't really alter anything else.

I agree with this. Let's make contributors' lives better as soon as we can, and iterate to the point where we are able to accomplish that for everyone.

@netweb I definitely agree that we need to dive into the issue and make it work on Windows too. But committing what we have at the moment will not make it harder for you to do what you were able to do before. Therefore I suggest doing a first commit now, but not closing the ticket with it.

Sorry but I disagree, we should ensure that the core committers who use Windows can use this first please, recent build tool changes have already caught out a lead developer (see #44262) because there was no Windows testing.

#11 @SergeyBiryukov
3 weeks ago

  • Description modified (diff)

#12 @SergeyBiryukov
3 weeks ago

Tested build-with-symlinks.patch on Windows -- grunt build:dev task works as expected, requires to be run as an administrator though for symlinks to be created properly (that's probably also to be expected).

Last edited 3 weeks ago by SergeyBiryukov (previous) (diff)

@omarreiss
3 weeks ago

Rebased

@omarreiss
2 weeks ago

Makes clean task a little bit less eager, cleans the symlinks before cleaning the files, shows better errors when symlinking fails.

#13 @SergeyBiryukov
2 weeks ago

Found an issue with the approach: when using symlinks, wp-load.php is included from the src/ directory, which means ABSPATH also points to src/, leading to a warning in the admin:

WARNING: S:\home\wordpress.test\develop\src\wp-includes\formatting.php:5434 - readfile(S:\home\wordpress.test\develop\src/wp-includes/js/wp-emoji-loader.js): failed to open stream: No such file or directory

The warning comes from _print_emoji_detection_script():

<?php readfile( ABSPATH . WPINC . '/js/wp-emoji-loader.js' ); ?>

At a glance, these functions are also affected:

  • rich_edit_exists():
    $wp_rich_edit_exists = file_exists( ABSPATH . WPINC . '/js/tinymce/tinymce.js' );
    
  • get_post_embed_html():
    $output .= file_get_contents( ABSPATH . WPINC . '/js/wp-embed.js' );
    
  • print_embed_scripts():
    readfile( ABSPATH . WPINC . '/js/wp-embed-template.js' );
    

ABSPATH is set as:

if ( ! defined( 'ABSPATH' ) ) {
	define( 'ABSPATH', dirname( __FILE__ ) . '/' );
}

Apparently __FILE__ always resolves symlinks. $_SERVER['SCRIPT_FILENAME'] could help, but it points to the main script (e.g. wp-admin/index.php), not the currently included file, so dirname( $_SERVER['SCRIPT_FILENAME'] ) does not give the correct path.

Creating a wp-config.php file in the build/ directory does not help either, it's simply ignored.

Patching wp-load.php is the only workaround I can think of at the moment:

if ( ! defined( 'ABSPATH' ) ) {
	define( 'ABSPATH', preg_replace( '#src$#', 'build', dirname( __FILE__ ) ) . '/' );
}

#14 follow-up: @SergeyBiryukov
2 weeks ago

  • It looks like files inside wp-admin are symlinked, but not the files in subfolders, e.g. wp-admin/includes, which are copied instead. Same for wp-includes, e.g. wp-includes/customize or wp-includes/rest-api.
  • The error in build:dev says: "Failed to delete symlinks". Seems like a copy/paste from clean-all, should probably be "Failed to create symlinks".

#15 in reply to: ↑ 14 @SergeyBiryukov
2 weeks ago

Replying to SergeyBiryukov:

It looks like files inside wp-admin are symlinked, but not the files in subfolders, e.g. wp-admin/includes, which are copied instead. Same for wp-includes, e.g. wp-includes/customize or wp-includes/rest-api.

Nevermind, missed that subfolders are symlinks themselves.

@omarreiss
13 days ago

Makes sure wp-load.php isn't symlinked because the file needs to be physically in the build folder.

#16 @SergeyBiryukov
13 days ago

I tried copying wp-load.php to build directory last night, but it didn't work in my testing.

build-with-symlinks4.patch doesn't seem to work either. The file does physically exist in the build/ folder now, but the files that include it (pretty much any core PHP file) still include the one from the src/ folder, as that's what require( dirname( __FILE__ ) . '/wp-load.php' ); resolves to.

Note: See TracTickets for help on using tickets.