Skip to content

Move some scripts from root directory to scripts/dev/#3589

Closed
petk wants to merge 1 commit intophp:masterfrom
petk:patch-scripts-1
Closed

Move some scripts from root directory to scripts/dev/#3589
petk wants to merge 1 commit intophp:masterfrom
petk:patch-scripts-1

Conversation

@petk
Copy link
Member

@petk petk commented Oct 6, 2018

At the time of this commit, there is a dedicated folder for development related tools and such scripts might fit better there to not bloat the project root directory too much.

@php-pulls
Copy link

Comment on behalf of petk at php.net:

Labelling

At the time of this commit, there is a dedicated folder for development
related tools and such scripts might fit better there to not bloat the
project root directory too much.

Move snapshot to scripts/dev/snapshot
@petk petk force-pushed the patch-scripts-1 branch from 81e3d8c to 6260f93 Compare October 6, 2018 19:56
@petk petk changed the title Move vcsclean script to scripts/dev/vcsclean Move some scripts from root directory to scripts/dev/ Oct 6, 2018
@nikic
Copy link
Member

nikic commented Oct 6, 2018

I have no idea what ./snapshot is for, but ./vcsclean is probably used in a bunch of existing scripts in 3rd party code. E.g. this call in gcov comes to mind: https://github.com/php/web-gcov/blob/89fe54d765e195876bbbdc6b95fd4bc1359f8960/cron/cron.sh#L111

Imho moving these scripts is not really worth the breakage.

@petk
Copy link
Member Author

petk commented Oct 6, 2018

The snapshot script is a helper to create an archive of the given php-src directory. Probably it was used with snaps.php.net and today isn't utilized anymore. In any case it probably should be refactored just a bit otherwise having it is OK, but not in the root directory. It touches a directory above the project root which is not expected behavior of such tool.

Few quick use cases of downloading PHP:

  • PHP downloaded from php.net for example, all these files also reside in it and none of these can't and shouldn't be used in such directory actually.
  • PHP cloned from a Git repository: all these are used in the development phase only or in gcov's case as a step to clean generated files (for automated builds and development)

At the gcov cron script specifically there is also git clean -xfd step called right after ./vcsclean which cleans the working tree even more for files that might be created but are not ignored.

Having such script in root directory is not only bloated to the end user, but also slightly tricky to not clean something unwanted (scripts located in a subdirectory are slightly more difficult to access at least). And not to mention, the vcsclean script is very simple helper for calling git clean -dfX...

Which all also comes also to next steps - the build/build.mk file targets gitclean-work (should be marked as .PHONY) and is used only for this vcsclean script purpose in the build process.

In case this is some sort of BC we can add it to the UPGRADING notes if it causes issues.

EDIT: Calling a non existing script from some other shell script doesn't cause any issues either:

#!/bin/sh
echo "step 1"
./calling-non-existant-script
echo "step 2"

^ this will still return exit code 0 and go to the step 2. It will emit a warning in the output...

@petk
Copy link
Member Author

petk commented Oct 6, 2018

I was also thinking the same for the makedist and genfiles scripts but these two would need two commits: one for moving them and one for refactoring them for usage ./scripts/dev/makedist...

@KalleZ
Copy link
Member

KalleZ commented Oct 6, 2018

As for the vcsclean script; wasn't this used by make clean?

@petk
Copy link
Member Author

petk commented Oct 6, 2018

make clean has manual steps defined here https://github.com/php/php-src/blob/master/Makefile.global#L112 and doesn't use this script.

@KalleZ
Copy link
Member

KalleZ commented Oct 6, 2018

Perfect then, just wanted to make sure =D

@nikic
Copy link
Member

nikic commented Feb 12, 2019

@petk Would you like to remove the vcsclean call in gcov? Afterwards this should be good for merge.

@petk
Copy link
Member Author

petk commented Feb 16, 2019

Yes, it will help a bit to people browsing the php-src repo and sources, I think.

Gcov has been fixed via php/web-gcov@1cf3ef1 and this pull request merged via e0c8803

Thanks!

@petk petk closed this Feb 16, 2019
@petk petk deleted the patch-scripts-1 branch February 16, 2019 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants