Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#44492 closed enhancement (fixed)

Add new --dev flag to allow building and cleaning /src again.

Reported by: omarreiss's profile omarreiss Owned by: atimmer's profile atimmer
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-testing
Focuses: Cc:

Description (last modified by omarreiss)

This ticket introduces a --dev flag to the build, copy, watch and clean tasks, meant specifically for development. This makes it possible to develop from /src again.

When running grunt build --dev we only run grunt build:js and grunt build:css. When running a regular grunt build we also include grunt build:files, which also copies over the PHP. This means we've also separated the different build concerns into different build tasks that could be run separately (extra win! 🎉).

We first tried an approach which used symlinking. However, this gave too many issues on too many systems so we reverted back to an approach which allows building into /src

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. More testing needed.

Attachments (9)

build-with-symlinks.patch (6.8 KB) - added by omarreiss 6 years ago.
build-with-symlinks2.patch (6.8 KB) - added by omarreiss 6 years ago.
Rebased
build-with-symlinks3.patch (14.1 KB) - added by omarreiss 6 years 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 6 years ago.
Makes sure wp-load.php isn't symlinked because the file needs to be physically in the build folder.
build-into-src.diff (35.4 KB) - added by atimmer 6 years ago.
build-into-src.2.diff (42.5 KB) - added by atimmer 6 years ago.
build-into-src.3.diff (42.7 KB) - added by atimmer 6 years ago.
build-into-src.4.diff (47.4 KB) - added by atimmer 6 years ago.
build-into-src.5.diff (41.3 KB) - added by atimmer 6 years ago.

Download all attachments as: .zip

Change History (43)

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


6 years ago

#2 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.0

#3 @SergeyBiryukov
6 years ago

  • Component changed from General to Build/Test Tools

#4 @netweb
6 years 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
6 years 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 6 years ago by flixos90 (previous) (diff)

#6 in reply to: ↑ 5 @netweb
6 years 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
6 years 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
6 years 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
6 years 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
6 years 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
6 years ago

  • Description modified (diff)

#12 @SergeyBiryukov
6 years ago

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

Version 0, edited 6 years ago by SergeyBiryukov (next)

@omarreiss
6 years ago

Rebased

@omarreiss
6 years ago

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

#13 @SergeyBiryukov
6 years 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
6 years 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
6 years 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
6 years ago

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

#16 @SergeyBiryukov
6 years 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.

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


6 years ago

#18 @jorbin
6 years ago

  • Milestone changed from 5.0 to 5.1

#19 @omarreiss
6 years ago

  • Description modified (diff)
  • Summary changed from Add new build:dev task which symlinks all files that can be symlinked to Add new --dev to allow building and cleaning /src again.

#20 @omarreiss
6 years ago

I've updated the description of this ticket to describe the new approach. What isn't covered yet by build-into-src.diff is that the index.php in /src still assumes WordPress can't be run from /src anymore. We need to come up with a different solution to signal devs they need to build.

#21 @SergeyBiryukov
6 years ago

  • Summary changed from Add new --dev to allow building and cleaning /src again. to Add new --dev flag to allow building and cleaning /src again.

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


6 years ago

#23 @atimmer
6 years ago

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

In 44359:

Build tools: Allow building WordPress to src.

After the JavaScript reorganization in [43309], it was no longer possible to test WordPress from the src folder. That meant a build step was required to test PHP modifications. That is suboptimal as even a simple copy is slower than a web server just serving the new file.

We achieve building to src by setting a WORKING_DIR constant in the Gruntfile that is build by default, but changes to src when the --dev flag is present on any Grunt command. We provide sensible defaults so some commands, such as copying version.php, always build to build.

Because testing from build is no longer required, we change the messages present in index.php and wp-admin/index.php to be more broadly about building WordPress.

We also change the webpack config to have more straightforward behavior based on the buildTarget argument. It only determines the build target now and has no implicit behavior anymore. grunt build still works as it worked before, to make sure that the build server produces the same wordpress.zip we are used to.

We do all this instead of a symlink setup because symlinks don't work on every platform.

Props omarreiss, netweb, flixos90, SergeyBiryukov.
Fixes #44492.

#24 @atimmer
6 years ago

In 44361:

Build tools: Fix the travis:js build.

After [44359] it is impossible to not use ES6 syntax for some logic in the Gruntfile.js, so adjust the esversion setting for the Gruntfile.js to 6. Because the previous setting in .jshintrc was not compatible with setting esversion, set the esversion in the .jshintrc explicitly.

See #44492.

#25 @iandunn
6 years ago

It seems like .gitignore should now have an entry for src/wp-admin/js/, is that correct?

#26 @SergeyBiryukov
6 years ago

In 44365:

Build Tools: After [44359], add src/wp-admin/js to .gitignore.

Props iandunn.
See #44492.

#27 @SergeyBiryukov
6 years ago

In 44366:

Build Tools: Remove unnecessary sprintf() in translatable strings.

Add a missing sprintf() for npm install substring.

See #44492.

#28 @atimmer
6 years ago

Thanks @SergeyBiryukov, I hadn't gotten around to it yet but now I don't have to 🙂

Thanks for spotting it @iandunn.

#29 @pento
6 years ago

In 44508:

Build/Tests: Default to running unit tests from src.

This commit also defaults WP-CLI commands to running against src, too.

Props atimmer, pento.
See #44492.
Fixes #45863.

#30 @pento
6 years ago

In 44509:

Build/Tests: Default to running unit tests from src.

This is the actual commit, unlike [44508], which was not.

Props atimmer.
See #44492.
Fixes #45863.

#31 @atimmer
6 years ago

@pento I don't have the capacity to look into this before Friday. As this is build tooling it shouldn't block beta1, right?

#32 @ocean90
5 years ago

In 45688:

I18N: Use RTL stylesheets when running from /src.

To run WordPress from /src you have to use the --dev flag which also builds the RTL stylesheets thus the admin notice and force to LTR is no longer required.

See #44492.
Fixes #44865.

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


5 years ago

#34 @ocean90
4 years ago

In 48311:

Administration: Make all color schemes available when running from /src.

To run WordPress from /src you have to use the --dev flag which also builds the color scheme stylesheets thus the restriction is no longer required.

See #44492.
Fixes #50558.

Note: See TracTickets for help on using tickets.