WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 3 months ago

Last modified 3 months ago

#52909 closed defect (bug) (fixed)

git repo README.md should include instructions to install svn

Reported by: czapla Owned by: desrosj
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: docs Cc:

Description

The README.md in the github repo should include instructions to install svn as part of the Development Environment set up.

The "env:install" npm script calls the install.js script which calls svn here: https://github.com/WordPress/wordpress-develop/blob/master/tools/local-env/scripts/install.js#L57

Attachments (1)

debug.log (2.4 KB) - added by alexstine 5 months ago.
debug.log from my local repo.

Download all attachments as: .zip

Change History (16)

#1 @desrosj
6 months ago

  • Component changed from General to Build/Test Tools
  • Keywords reporter-feedback added

HI @czapla,

Thanks for this ticket, and welcome to Trac!

Are you seeing an error when SVN is not installed when running env:install? This command should be instructing Docker to execute the command within the local Docker development environment, which does have SVN installed.

If the command is not working correctly and producing an error, we'll want to figure that out.

#2 follow-up: @czapla
6 months ago

Hi @desrosj !

Thanks for the welcome! My first WordPress ticket as you can tell 😊

Are you seeing an error when SVN is not installed when running env:install?

That's correct.

This command should be instructing Docker to execute the command within the local Docker development environment, which does have SVN installed.

Oh, in that case, I believe that the whole command should be passed to the shell with sh -c or the second command should be prefaced with docker-compose exec -T <container> like:

docker-compose exec -T php svn checkout -r <rest of the command...>

This is per: https://docs.docker.com/engine/reference/commandline/exec/#extended-description

COMMAND should be an executable, a chained or a quoted command will not work. Example: docker exec -ti my_container "echo a && echo b" will not work, but docker exec -ti my_container sh -c "echo a && echo b" will.

(These are the docs for the docker command, but it works the same for docker-compose)

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


5 months ago

#4 @desrosj
5 months ago

Just noting, this was introduced in #49720.

@alexstine
5 months ago

debug.log from my local repo.

#5 @alexstine
5 months ago

I experienced an error where svn returned command not found killing npm run env:install. I've attached my debug.log.

Hope it helps.

#6 in reply to: ↑ 2 @jnylen0
5 months ago

  • Keywords reporter-feedback removed

Replying to czapla:

This command should be instructing Docker to execute the command within the local Docker development environment, which does have SVN installed.

Oh, in that case, I believe that the whole command should be passed to the shell with sh -c or the second command should be prefaced with docker-compose exec -T <container> like:

This is correct. execSync( 'docker-compose exec -T commandA && commandB' ) will effectively run two commands, docker-compose exec -T commandA and commandB, because the && is interpreted as a command separator by the underlying shell, and never gets passed to docker-compose.

Also it seems like this may not work in Windows if it is supposed to (&& is a Linux-ism). A good way to restructure this could be as follows (needs testing):

execSync( `docker-compose exec -T php rm -rf ${test_plugin_directory}`, { stdio: 'inherit' } );
execSync( `docker-compose exec -T php svn checkout -r ${process.env.WP_IMPORTER_REVISION} https://plugins.svn.wordpress.org/wordpress-importer/trunk/ ${test_plugin_directory}`, { stdio: 'inherit' } );
Version 0, edited 5 months ago by jnylen0 (next)

#7 @desrosj
5 months ago

#53133 was marked as a duplicate.

#8 @desrosj
5 months ago

  • Milestone changed from Awaiting Review to 5.8

I did some digging on this today and it appears I was mistaken when I said the Docker container had SVN installed. There are a few options to fix this.

  1. Add SVN to the containers and use the example from @jnylen0 above.

I don't love this. The containers are getting pretty large, so I'm hesitating to install another package when there could be a better solution. With the exception of this script, I don't think anyone will be looking to use SVN commands directly within the container after running the install script.

  1. Switch to using Git (which is installed on the containers)

This would allow us to continue targeting a specific revision to use in testing. This helps when changes are made that are not released for a period of time.

  1. Make the Importer available via Composer.

This would allow wordpress/importer to be specified in the composer.json file.

  1. Use a URL to download and unzip the plugin.

With options 3 & 4, the ability to target specific revisions may potentially be lost as those methods would be tied to specific versions.

#9 @jnylen0
5 months ago

There is another option: bundle the importer plugin with the source tree, which eliminates this step entirely. This is the path that ClassicPress has taken.

This adds another infrequent maintenance task but makes things easier and quicker for contributors setting up a development environment. Also, the importer plugin is not updated very often.

#10 @alexstine
5 months ago

My vote would be for option 2. If the WordPress Importer is ever updated, and it needs some updates, would be easier for testing this way. Anyway, any option is better than just a complete fail.

Thanks.

#11 @desrosj
4 months ago

  • Keywords needs-patch added

5.8 beta deadline is today, but this only affects build/test tools and changes are allowed during the beta cycle. I'd like to resolve this during this cycle, so leaving this in the milestone a bit longer.

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


3 months ago

  • Keywords has-patch added; needs-patch removed

#13 @desrosj
3 months ago

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

In 51179:

Build/Test Tools: Use Git when fetching the WordPress Importer for use in tests.

This switches to using Git in the local Docker environment install script to check out a copy of the WordPress Importer plugin for use in unit tests.

Previously, SVN was used and the commands were not correctly run within the Docker container. The container does not actually have SVN installed, and the script was only working when the machine running the command had SVN present.

Props czapla, alexstine, jnylen0, francina, desrosj.
Fixes #52909.

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


3 months ago

Note: See TracTickets for help on using tickets.