Build: Prepare tooling for Gutenberg extensions in Jetpack #11639

sirreal · 2019-03-21T13:10:29Z

Adds files from Jetpack Gutenberg extensions folder (about to be merged in #11633) to ignore lists relevant for the packaged version of the plugin.

Changes proposed in this Pull Request:

SVN Ignores
CDN asset ignores
Ignores for release branches

Testing instructions:

Together with #11633 (git merge --squash --no-commit origin/add/jetpack-blocks-src) — does the build ignore assets in /extensions/* but still sees *.php files?

yarn build-production
php bin/build-asset-cdn-json.php

An alternative way of testing this would be to:

Checkout this branch.
Add the blocks src to: git merge --squash --no-commit origin/add/jetpack-blocks-src
Comment out this line so we can build a release branch from our current branch:

jetpack/tools/build-release-branch.sh

Line 102 in e450f01

git checkout master && git pull && git reset --hard origin/master

You can apply this patch:
https://gist.github.com/sirreal/2245987078998c2b6b9f95ba45d93d61#file-tooling-patch
Commit the changes (git add -A && git commit -am 'DROPME: testing changes')
Run tools/build-release-branch.sh -n
Pick a unique branch name.
Let the build happen for both branches (the release branch and the built branch)
Once the build is over, check the built branch
It should not include any of the block's source files.
the .gitignore should include the new lines.

Proposed changelog entry for your changes:

None

.gitattributes

bin/build-asset-cdn-json.php

.svnignore

simison · 2019-03-21T14:17:29Z

.svnignore

@@ -42,3 +42,4 @@ docker
 bin/pre-commit-hook.js
 bin/travis_install.sh
 yarn-error.log
+extensions/**/*.{js,jsx,json,gif,png,css,jpg,jpeg,sass,scss}


Copying over #11639 (comment) for visibility:

To help future groking, I'd love for svnignore comments (I believe # preceding the line should work here too) to explain why these files are ignored. Someone new to JP development may be confused why we're ignoring jpg, css, etc.

suggested by @dereksmart, could we do everything except php instead?

See #11639 (comment):

I'd rather be explicit in svnignore as it can be really surprising. Something works great in GItHub and unless someone tests that explicit feature using a beta that was shipped in SVN we could have a pretty big gap in the production release.

svnignore is not a real thing. Under the hood, this is relying on shell globbing:

jetpack/tools/build-jetpack.sh

Lines 70 to 75 in f8078c2

for file in $( cat "$TARGET_DIR/.svnignore" 2>/dev/null ); do

if [[ $file == "to-test.md" || $file == "docs/testing/testing-tips.md" ]]; then

continue

fi

rm -rf $TARGET_DIR/$file

done

We should be extremely careful and keep that in mind.

jeherve

I think we'll need to add the files here too:

jetpack/tools/build-release-branch.sh

Line 53 in 36a8543

# Add custom stuff to .gitignore release

.svnignore

simison · 2019-03-22T10:56:07Z

modules/photon-cdn/jetpack-manifest.php

-  500 => 'modules/lazy-images/js/lazy-images.js',
-  501 => 'modules/wpgroho.js',
-);
+<?php 


@jeherve @kraftbj do we need to generate this file here or you'll do this during the build when packaging the release?

It's done automatically during the release process.

jetpackbot · 2019-03-22T11:02:34Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against ba75320

bin/build-asset-cdn-json.php

addressed

modules/photon-cdn/jetpack-manifest.php

tools/build-release-branch.sh

bin/build-asset-cdn-json.php

ockham · 2019-03-22T13:20:59Z

Testing instructions:

Together with #11633 — does the build ignore assets in /extensions/* but still sees *.php files?

yarn build-production

php bin/build-asset-cdn-json.php

Where do I check for presence of *.php files exactly? I opened modules/photon-cdn/jetpack-manifes.php, but that file doesn't reference any PHP files... 😬

ockham · 2019-03-25T11:37:47Z

tools/build-release-branch.sh

+	echo "/extensions/**/*.sass" >> .gitignore
+	echo "/extensions/**/*.scss" >> .gitignore
+	echo "/extensions/**/*.svg" >> .gitignore
+	echo "__snapshots__/" >> .gitignore


Late to the .gitignore party, but would a (semi-)whitelist-based approach (i.e. using negation) work? Something like

echo "/extensions/" >> .gitignore echo "!/extensions/**/*.php" >> .gitignore

See e.g. https://www.atlassian.com/git/tutorials/saving-changes/gitignore#git-ignore-patterns

That would likely work, but would be harder to keep in sync with the .svnignore which is really a list of files to pass to rm -fr.

why we need a .svnignore again?

It's used in several places around the build/deploy tooling:

jetpack/tools/build-jetpack.sh

Lines 70 to 75 in f8078c2

for file in $( cat "$TARGET_DIR/.svnignore" 2>/dev/null ); do

if [[ $file == "to-test.md" || $file == "docs/testing/testing-tips.md" ]]; then

continue

fi

rm -rf $TARGET_DIR/$file

done

jetpack/tools/build-release-branch.sh

Lines 228 to 235 in 58ec303

for file in $( cat "$DIR/.svnignore" 2>/dev/null ); do

# We want to commit changes to to-test.md as well as the testing tips.

if [[ $file == "to-test.md" || $file == "docs/testing/testing-tips.md" ]]

then

continue;

fi

rm -rf TMP_LOCAL_BUILT_VERSION/$file

done

jetpack/tools/deploy-to-master-stable-branch.sh

Lines 67 to 73 in f8078c2

for file in $( cat "$JETPACK_GIT_DIR/.svnignore" 2>/dev/null ); do

# We want to commit changes to to-test.md as well as the testing tips.

if [ $file == "to-test.md" || $file == "docs/testing/testing-tips.md" ]; then

continue;

fi

rm -rf $JETPACK_TMP_DIR_2/$file

done

jetpack/tools/deploy-to-svn.sh

Lines 72 to 74 in cadfa61

for file in $( cat "$JETPACK_GIT_DIR/.svnignore" 2>/dev/null ); do

rm -rf $JETPACK_SVN_DIR/trunk/$file

done

Not for this PR, but I wonder if we could de-dupe sources of truth (i.e. use .gitifnore where we're now using .svnignore) in the long run... 🤔

ockham · 2019-03-25T11:41:09Z

tools/build-release-branch.sh

+
+			git rm -r --cached .
+			git add .
+			git commit -m ".gitignore cleanup"


Maybe make the commit msg more explicit here? Like Remove files based on .gitignore contents?

ockham · 2019-03-25T11:41:45Z

tools/build-release-branch.sh

+			modify_svnignore
+
+			git rm -r --cached .
+			git add .


Do we need this line? Not seeing anything added by that commit 🤔

I think the logic here is

Remove everything

Add everything (respecting our new .gitignore)

This is the way we're able to filter out versioned files that are newly added to the gitignore.

ockham

Left a few minor (non-blocking) suggestions. Overall, this is looking great and working well AFAICT!

🚢 at will!

jeherve

While this appears to work for now, I have some reservations about the latest changes; they can potentially be dangerous in that you can end up committing things you did not mean to.

tools/build-release-branch.sh

sirreal · 2019-03-25T14:33:42Z

I've added fdaeab9 and 327218f to address the latest review.

sirreal · 2019-03-25T16:45:09Z

After a call with @kraftbj and @dereksmart representing @Automattic/jetpack-crew, I've rolled back the changes that were intended to remove many files from extensions in release branches.

Part of the reasoning is that other unbuilt source is already included in release branches so including source in beta branches is not a regression and not something that needs to be addressed in this PR.

Thanks everyone, reviews appreciated 🙇

Stale

ockham

Seems to work as described and expected 👍

kraftbj

I'm cautious about the additions in .svnignore and wanting to make sure they work as expected, so this will be an area of heavy testing in the next couple of days.

…11639)" This reverts commit 0d14622.

* Revert "Add source for Gutenberg extensions (#11633)" This reverts commit a2bf967. * Revert "Build: Prepare tooling for Gutenberg extensions in Jetpack (#11639)" This reverts commit 0d14622.

sirreal requested a review from simison March 21, 2019 13:14

simison reviewed Mar 21, 2019

View reviewed changes

.gitattributes Outdated Show resolved Hide resolved

simison reviewed Mar 21, 2019

View reviewed changes

bin/build-asset-cdn-json.php Outdated Show resolved Hide resolved

simison added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Mar 21, 2019

simison requested review from simison and a team March 21, 2019 13:18

sirreal commented Mar 21, 2019

View reviewed changes

.svnignore Outdated Show resolved Hide resolved

jeherve added [Type] Janitorial [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Mar 21, 2019

simison mentioned this pull request Mar 21, 2019

Extensions: Add block editor (Gutenberg) extensions source #11633

Merged

simison reviewed Mar 21, 2019

View reviewed changes

jeherve previously requested changes Mar 21, 2019

View reviewed changes

simison reviewed Mar 21, 2019

View reviewed changes

.svnignore Outdated Show resolved Hide resolved

simison reviewed Mar 22, 2019

View reviewed changes

simison marked this pull request as ready for review March 22, 2019 10:57

simison reviewed Mar 22, 2019

View reviewed changes

bin/build-asset-cdn-json.php Outdated Show resolved Hide resolved

simison reviewed Mar 22, 2019

View reviewed changes

bin/build-asset-cdn-json.php Outdated Show resolved Hide resolved

simison reviewed Mar 22, 2019

View reviewed changes

bin/build-asset-cdn-json.php Outdated Show resolved Hide resolved

sirreal changed the title ~~Update/prep tooling for block src~~ Build: Prepare tooling for Gutenberg extensions in Jetpack Mar 22, 2019

sirreal requested review from simison and a team March 22, 2019 12:07

sirreal commented Mar 22, 2019

View reviewed changes

modules/photon-cdn/jetpack-manifest.php Outdated Show resolved Hide resolved

ockham reviewed Mar 22, 2019

View reviewed changes

tools/build-release-branch.sh Outdated Show resolved Hide resolved

ockham reviewed Mar 22, 2019

View reviewed changes

bin/build-asset-cdn-json.php Show resolved Hide resolved

ockham reviewed Mar 22, 2019

View reviewed changes

bin/build-asset-cdn-json.php Outdated Show resolved Hide resolved

sirreal removed the request for review from simison March 22, 2019 14:05

Add docker to release branch gitignore

28ade40

ockham reviewed Mar 25, 2019

View reviewed changes

ockham previously approved these changes Mar 25, 2019

View reviewed changes

jeherve previously requested changes Mar 25, 2019

View reviewed changes

tools/build-release-branch.sh Outdated Show resolved Hide resolved

tools/build-release-branch.sh Outdated Show resolved Hide resolved

sirreal added 2 commits March 25, 2019 15:32

Remove undesired files explicitly

fdaeab9

Improve gitignore message

327218f

sirreal dismissed ockham’s stale review via 327218f March 25, 2019 14:32

sirreal requested a review from jeherve March 25, 2019 14:33

sirreal requested a review from a team March 25, 2019 14:38

kraftbj added this to the 7.2 milestone Mar 25, 2019

Revert source files removal

ba75320

sirreal requested review from a team and removed request for jeherve March 25, 2019 16:45

ockham approved these changes Mar 25, 2019

View reviewed changes

kraftbj approved these changes Mar 25, 2019

View reviewed changes

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 25, 2019

sirreal merged commit 0d14622 into master Mar 25, 2019

matticbot added the [Status] Needs Changelog label Mar 25, 2019

ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 25, 2019

sirreal deleted the update/prep-tooling-for-block-src branch March 25, 2019 17:24

kraftbj removed the [Status] Needs Changelog label Mar 25, 2019

kraftbj added a commit that referenced this pull request Mar 26, 2019

Revert "Build: Prepare tooling for Gutenberg extensions in Jetpack (#…

d19ac00

…11639)" This reverts commit 0d14622.

kraftbj mentioned this pull request Mar 26, 2019

Extensions: Add block editor extensions source #11685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: Prepare tooling for Gutenberg extensions in Jetpack #11639

Build: Prepare tooling for Gutenberg extensions in Jetpack #11639

sirreal commented Mar 21, 2019 •

edited

Loading

simison Mar 21, 2019

lezama Mar 21, 2019

simison Mar 21, 2019 •

edited

Loading

sirreal Mar 22, 2019

jeherve left a comment

simison Mar 22, 2019 •

edited

Loading

jeherve Mar 22, 2019

jetpackbot commented Mar 22, 2019 •

edited

Loading

ockham commented Mar 22, 2019

Testing instructions:

ockham Mar 25, 2019

sirreal Mar 25, 2019

lezama Mar 25, 2019

sirreal Mar 25, 2019

ockham Mar 25, 2019

ockham Mar 25, 2019

ockham Mar 25, 2019

sirreal Mar 25, 2019

ockham left a comment

jeherve left a comment

sirreal commented Mar 25, 2019

sirreal commented Mar 25, 2019

ockham left a comment

kraftbj left a comment

	for file in $( cat "$TARGET_DIR/.svnignore" 2>/dev/null ); do
	if [[ $file == "to-test.md" \|\| $file == "docs/testing/testing-tips.md" ]]; then
	continue
	fi
	rm -rf $TARGET_DIR/$file
	done

	for file in $( cat "$DIR/.svnignore" 2>/dev/null ); do
	# We want to commit changes to to-test.md as well as the testing tips.
	if [[ $file == "to-test.md" \|\| $file == "docs/testing/testing-tips.md" ]]
	then
	continue;
	fi
	rm -rf TMP_LOCAL_BUILT_VERSION/$file
	done

	for file in $( cat "$JETPACK_GIT_DIR/.svnignore" 2>/dev/null ); do
	# We want to commit changes to to-test.md as well as the testing tips.
	if [ $file == "to-test.md" \|\| $file == "docs/testing/testing-tips.md" ]; then
	continue;
	fi
	rm -rf $JETPACK_TMP_DIR_2/$file
	done

	for file in $( cat "$JETPACK_GIT_DIR/.svnignore" 2>/dev/null ); do
	rm -rf $JETPACK_SVN_DIR/trunk/$file
	done

Build: Prepare tooling for Gutenberg extensions in Jetpack #11639

Build: Prepare tooling for Gutenberg extensions in Jetpack #11639

Conversation

sirreal commented Mar 21, 2019 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

simison Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetpackbot commented Mar 22, 2019 • edited Loading

ockham commented Mar 22, 2019

Testing instructions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

sirreal commented Mar 25, 2019

sirreal commented Mar 25, 2019

ockham left a comment

Choose a reason for hiding this comment

kraftbj left a comment

Choose a reason for hiding this comment

sirreal commented Mar 21, 2019 •

edited

Loading

simison Mar 21, 2019 •

edited

Loading

simison Mar 22, 2019 •

edited

Loading

jetpackbot commented Mar 22, 2019 •

edited

Loading