#44492 closed enhancement (fixed)
Add new --dev flag to allow building and cleaning /src again.
Reported by: | omarreiss | Owned by: | atimmer |
---|---|---|---|
Milestone: | 5.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | needs-testing |
Focuses: | Cc: |
Description (last modified by )
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)
Change History (43)
This ticket was mentioned in Slack in #core-committers by omarreiss. View the logs.
6 years ago
#5
follow-up:
↓ 6
@
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, 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.
#6
in reply to:
↑ 5
@
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:
↓ 8
@
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
@
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:
↓ 10
@
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
@
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.
#12
@
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 for symlinks to be created properly (that's probably also to be expected).
@
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
@
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:
↓ 15
@
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 forwp-includes
, e.g.wp-includes/customize
orwp-includes/rest-api
. - The error in
build:dev
says: "Failed to delete symlinks". Seems like a copy/paste fromclean-all
, should probably be "Failed to create symlinks".
#15
in reply to:
↑ 14
@
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 forwp-includes
, e.g.wp-includes/customize
orwp-includes/rest-api
.
Nevermind, missed that subfolders are symlinks themselves.
@
6 years ago
Makes sure wp-load.php isn't symlinked because the file needs to be physically in the build folder.
#16
@
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
#19
@
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
@
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
@
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
@
6 years ago
- Owner set to atimmer
- Resolution set to fixed
- Status changed from new to closed
In 44359:
#25
@
6 years ago
It seems like .gitignore
should now have an entry for src/wp-admin/js/
, is that correct?
#28
@
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.
#31
@
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?
Looking at https://www.npmjs.com/package/grunt-contrib-symlink#usage-tips-on-microsoft-windows:
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.