Build: Do not declare symlink strategy for the Jetpack Logo package #12480

oskosk · 2019-05-28T19:32:19Z

When bundling the plugin, we remove packages so symlinks are not working there.

This will allow us to work locally with symlinked packages, but rely on the environment variable COMPOSER_MIRROR_PATH_REPOS for build scripts

Check the description of COMPOSER_MIRROR_PATH_REPOS in Composer environment variables.
Confirm that the default path strategy is symlink.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

The latter

Testing instructions:

check this branch
Remove the vendor directory
run COMPOSER_MIRROR_PATH_REPOS=1 composer install and confirm the jetpack-logo package is mirroed instead of symlinked.

Proposed changelog entry for your changes:

None needed

jetpackbot · 2019-05-28T19:34:17Z

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: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against 915ce04

simison · 2019-05-29T05:24:09Z

packages/logo/composer.json

 	"name": "automattic/jetpack-logo",
 	"description": "A logo for Jetpack",
 	"type": "library",
+	"version": "1.0.0",


Note:

When you add a hardcoded version to a VCS, the version will conflict with tag names. Composer will not be able to determine the version number.

via https://getcomposer.org/doc/02-libraries.md#library-versioning (edited)

and

The version of the package. In most cases this is not required and should be omitted
…
Specifying the version yourself will most likely end up creating problems at some point due to human error.

via

https://getcomposer.org/doc/04-schema.md#version

In the publishing scripts, you might actually want to validate that this isn't here.

tyxla

COMPOSER_MIRROR_PATH_REPOS=1 is a nice find 👍 I'd advise against declaring version in composer.json, but we need to change the stability constraint in the main composer.json. I'll handle that.

tyxla · 2019-05-29T06:24:03Z

composer.json

 			"type": "path",
-			"url": "./packages/logo",
-			"options": {
-				"symlink": true


tyxla · 2019-05-29T06:25:52Z

packages/logo/composer.json

 	"name": "automattic/jetpack-logo",
 	"description": "A logo for Jetpack",
 	"type": "library",
+	"version": "1.0.0",


By all means, I'd prefer not having to specify this. After all, currently we only need it so composer locally can find the package and symlink/mirror it. Let's avoid that by setting a lower minimum stability for the package (as we don't care about stability - we'll always use the local package). That can happen by specifying @dev as the version/stability constraint.

tyxla · 2019-05-29T06:26:26Z

tools/build-jetpack.sh

 }
 yarn --cwd $TARGET_DIR cache clean
-yarn --cwd $TARGET_DIR run build
+COMPOSER_MIRROR_PATH_REPOS=1 yarn --cwd $TARGET_DIR run build


👍 This seems to work well for me:

Installing automattic/jetpack-logo (dev-update/dont-symlink-packages): Mirroring from ./packages/logo

while without it for local dev I properly get:

Installing automattic/jetpack-logo (dev-update/dont-symlink-packages): Symlinking from ./packages/logo

tyxla · 2019-05-29T06:30:22Z

composer.lock

        {
            "name": "automattic/jetpack-logo",
-            "version": "1.0.0",
+            "version": "dev-update/dont-symlink-packages",


@oskosk we'll need to build a new composer.lock when in the master branch to avoid this reference.

tyxla · 2019-05-29T06:34:50Z

I've just:

Removed the version again from the logo package composer.json
Updated the stability constraint for the logo package to @dev so Composer wouldn't care about a particular version.
Rebuilt the composer.lock (although it'll need another rebuild on master)

Seems to work well for me both in dev and prod, but it's worth getting some @Automattic/jetpack-crew 👀 to test it better.

jeherve

This seems to be working now. 🚢 it!

Maybe in a next PR we could consider removing COMPOSER_MIRROR_PATH_REPOS=1 composer install --working-dir $TARGET_DIR altogether, as @oskosk suggested, since we do run composer install later on as part of yarn build.

tyxla

Tests well for me 👍

…12480) * Do not declare symlink strategy * Update bundling scripts to rely on COMPOSER_MIRROR_PATH_REPOS * Lower stability constraint of logo package * Update composer.lock * Add COMPOSER_MIRROR_PATH_REPOS=1 to composer install too * Fix composer install in build jetpack script

jeherve · 2019-05-29T11:35:10Z

Cherry-picked to branch-7.4 in f008508

Related: #12480

oskosk added [Status] Needs Review This PR is ready for review. Build [Focus] Jetpack DNA labels May 28, 2019

oskosk requested a review from a team May 28, 2019 19:32

oskosk force-pushed the update/dont-symlink-packages branch 2 times, most recently from c961853 to f445ba5 Compare May 28, 2019 20:15

lezama previously approved these changes May 28, 2019

View reviewed changes

oskosk dismissed lezama’s stale review via 72307d1 May 28, 2019 21:09

lezama previously approved these changes May 28, 2019

View reviewed changes

jeherve added this to the 7.4 milestone May 28, 2019

jeherve added [Pri] BLOCKER [Status] Needs Cherry-Pick labels May 28, 2019

jeherve assigned oskosk May 28, 2019

oskosk added 2 commits May 28, 2019 18:47

Declare version in logo package

ac66e56

Do not declare symlink strategy

8d45e75

oskosk dismissed lezama’s stale review via 8d45e75 May 28, 2019 21:48

oskosk force-pushed the update/dont-symlink-packages branch from 72307d1 to 8d45e75 Compare May 28, 2019 21:48

oskosk changed the title ~~Build: Do not symlink Jetpack Logo package for now~~ Build: Do not declare symlink strategy for the Jetpack Logo package May 28, 2019

Update bundling scripts to rely on COMPOSER_MIRROR_PATH_REPOS

e6c6874

lezama previously approved these changes May 28, 2019

View reviewed changes

simison reviewed May 29, 2019

View reviewed changes

tyxla reviewed May 29, 2019

View reviewed changes

tyxla added 3 commits May 29, 2019 09:27

Remove version from logo package

3263db1

Lower stability constraint of logo package

976e82f

Update composer.lock

3733281

tyxla dismissed lezama’s stale review via 3733281 May 29, 2019 06:29

tyxla reviewed May 29, 2019

View reviewed changes

tyxla mentioned this pull request May 29, 2019

Composer: Define local packages with a wildcard #12483

Merged

tyxla mentioned this pull request May 29, 2019

Add Custom Autoloader #12447

Merged

tyxla added 2 commits May 29, 2019 11:11

Add COMPOSER_MIRROR_PATH_REPOS=1 to composer install too

3b4cb7c

Fix composer install in build jetpack script

915ce04

jeherve approved these changes May 29, 2019

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 29, 2019

tyxla approved these changes May 29, 2019

View reviewed changes

oskosk merged commit 003372a into master May 29, 2019

matticbot added the [Status] Needs Changelog label May 29, 2019

oskosk deleted the update/dont-symlink-packages branch May 29, 2019 11:34

matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 29, 2019

jeherve removed [Status] Needs Changelog [Status] Needs Cherry-Pick labels May 29, 2019

jeherve added a commit that referenced this pull request Sep 16, 2019

Build Workflow: rely on COMPOSER_MIRROR_PATH_REPOS for packages

2617f9d

Related: #12480

jeherve mentioned this pull request Sep 16, 2019

Build Workflow: rely on COMPOSER_MIRROR_PATH_REPOS for packages #13488

Merged

dereksmart pushed a commit that referenced this pull request Sep 16, 2019

Build Workflow: rely on COMPOSER_MIRROR_PATH_REPOS for packages (#13488)

c0f615d

Related: #12480

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Build: Do not declare symlink strategy for the Jetpack Logo package #12480

Build: Do not declare symlink strategy for the Jetpack Logo package #12480

Uh oh!

oskosk commented May 28, 2019 •

edited

Loading

Uh oh!

jetpackbot commented May 28, 2019 •

edited

Loading

Uh oh!

simison May 29, 2019 •

edited

Loading

Uh oh!

tyxla left a comment

Uh oh!

tyxla May 29, 2019

Uh oh!

tyxla May 29, 2019

Uh oh!

tyxla May 29, 2019

Uh oh!

tyxla May 29, 2019

Uh oh!

tyxla commented May 29, 2019

Uh oh!

jeherve left a comment

Uh oh!

tyxla left a comment

Uh oh!

jeherve commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Build: Do not declare symlink strategy for the Jetpack Logo package #12480

Build: Do not declare symlink strategy for the Jetpack Logo package #12480

Uh oh!

Conversation

oskosk commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

jetpackbot commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simison May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

tyxla May 29, 2019

Choose a reason for hiding this comment

Uh oh!

tyxla May 29, 2019

Choose a reason for hiding this comment

Uh oh!

tyxla May 29, 2019

Choose a reason for hiding this comment

Uh oh!

tyxla May 29, 2019

Choose a reason for hiding this comment

Uh oh!

tyxla commented May 29, 2019

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

jeherve commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

oskosk commented May 28, 2019 •

edited

Loading

jetpackbot commented May 28, 2019 •

edited

Loading

simison May 29, 2019 •

edited

Loading