link: override dependency versions with the current dependent package #51

schomatis · 2018-08-29T14:56:10Z

This PR extracts the --sync feature from #48 (now named --override-deps) without the vendor/ support (that has some implementation problems, see #46 (comment)).

As with the imported PR, this command now requires gx-go link to be run inside the parent package of the dependency even though the link is still done to the global workspace. This breaks API but it makes it easier to reason about the code and about the link being done, and in most use cases the developer was already calling the command from a package anyway.

Closes #46.

link: add feature to override dependencies with linked packages

(This patch changes the current API.)

Add `--override-deps` flag to (in the case a of a dependency version mismatch)
replace the dependency version of the target package with the one in the current
dependent package. (Most of the implementation logic for this feature had to be
added to the `post-install` hook which also has a new `--override-deps` flag.)

Require the command to be run inside a package. Refactor the link/unlink
functions. Give more visibility to the options that allows specifying a
dependency to link by its name.

schomatis · 2018-08-29T16:24:57Z

@Stebalien PTAL. Although sub-optimal this still seems like a useful flag while developing across packages (especially for new developers who may not be familiarized with the gx-workspace and gx-update workflows).

schomatis · 2018-08-29T16:33:26Z

(Another possible enhancement for this PR or a new one: detect already linked packages and un-link+link them again. At the moment it would seem that the post-install hook + rewrite functionality doesn't re-process an already rewritten file -it's assuming that the import paths wont change?, but if we've updated something?- so if I link without --sync and it fails due to a mismatch version, when I link again, now with --sync, the rewrite part of the process won't happen but it won't fail either. The result is that both with and without the sync functionality will seem to fail -the user will need to actually un-link and re-link manually, but this won't be obvious to him/her-.)

Stebalien · 2018-08-29T21:17:41Z

link.go

@@ -53,21 +57,33 @@ unlinked QmVGtdTZdTFaLsaj2RwdVG8jcjNNcp1DE914DKZ2kHmXHw /home/user/go/src/github
 			Name:  "a,all",
 			Usage: "Remove all existing symlinks and reinstate the gx packages. Use with -r.",
 		},
+		cli.StringFlag{
+			Name:  "p,parent",


I'd leave this out. We can consider adding a -C flag (like git) later but that's leave that until we need it (and then make it a global option).

Stebalien · 2018-08-29T21:18:45Z

link.go

@@ -19,30 +19,34 @@ var LinkCommand = cli.Command{
 	Usage: "Symlink packages to their dvcsimport repos, for local development.",
 	Description: `gx-go link eases local development by symlinking actual workspace repositories on demand.

-Example workflow:
+Links the specified dependency package (by name or hash) used by the
+parent package (in the CWD or specified by --parent). The link is done


I'd just say "the current" package and drop the "parent" package language.

I'd prefer to maintain the parent-child relationship concept, because everything is a package here but, depending on where you're looking from (in what part of the code you're in), that package is either a dependency (and that distinction is important because a dependency in a package.json doesn't have the same information as the actual package) or the package that's importing that dependency. The term "current" doesn't seem to carry that relationship meaning, is there another term you'd prefer?

When I'm using GX, I'm usually working on a gx package. The "current" package is the package I'm currently working on.

For example:

gx-go link replaces the target gx package (either by name or hash) with a symlink to the appropriate dvcs repo in your GOPATH. To make this "symlinked" repo usable as a gx package, gx-go link rewrites the target package's dvcs imports using the target package's package.json. Unfortunately, this can cause build errors in packages depending on this package if these dependent packages specify alternative, conflicting dependency versions. We can work around this by asking gx to rewrite the target package using dependencies from the current package (the package you're trying to build) first, falling back on the target package's package.json file. We can do this by passing the --deps=mine flag.

That is a bit wordy and you're right, "current" may not be quite right.

Let's leave it at "the current dependent package". Nice text (I like it wordy :), I'll add it to the usage doc.

Stebalien · 2018-08-29T21:19:13Z

link.go

-linked QmVGtdTZdTFaLsaj2RwdVG8jcjNNcp1DE914DKZ2kHmXHw /home/user/go/src/github.com/multiformats/go-multihash
-linked QmSpJByNKFX1sCsHBEp3R73FL4NF6FnQTEGyNAXHm2GS52 /home/user/go/src/github.com/ipfs/go-log
+If the dependency versions of the parent package don't match the ones in
+the dependency being linked use the --sync flag to resolve the conflicts


"linked, use the"

Stebalien · 2018-08-29T21:22:57Z

link.go

+linked package). Note that this won't sync dependencies between different
+packages being linked. (Note: Make sure all the dependencies of the parent
+package are available to build its rewrite map if using --sync, e.g., by
+calling 'gx install --global'.)


This has a lot of parentheses. It'll probably read better if we're more direct. I.e., instead of saying "resolve the conflicts (...how it does it...", just say what it does.

Stebalien · 2018-08-29T21:24:19Z

link.go

+		},
+		cli.BoolFlag{
+			Name:  "s,sync",
+			Usage: "Synchronize the dependencies of the linked package with the ones of its parent.",


Again, I'd like to get rid of the "parent" terminology (packages have reverse-dependencies and dependent packages but they don't really have "parents").

Would you prefer to use "dependent"?

I would, but then you have to answer the question "which dependent". The answer is "the current one".

Stebalien · 2018-08-29T21:26:38Z

link.go

+			Usage: "Specify the path of the parent package of the linked dependency (default: CWD).",
+		},
+		cli.BoolFlag{
+			Name:  "s,sync",


"sync" feels like like the wrong term here (feels bidirectional). Maybe "--override-deps"? We could even go for --deps=mine (eventually adding --deps=theirs).

Yes, I don't really like "sync" but couldn't come up with something better. Let's go with --deps= and use the term that will replace "parent" here, e.g., --deps=<package-that-is-including-this-package>.

Stebalien · 2018-08-30T04:35:00Z

detect already linked packages and un-link+link them again.

We should probably do that but we can do it in a new patch.

Stebalien · 2018-08-30T21:55:53Z

Thinking about this, we could actually use the dependencies specified in the original gxed package (the one we're replacing with a symlink). That would be the most conservative approach (effectively "downgrading" dependencies.

The only difference is that, if the target repo has added a new dependency also present in the current/parent package, we'll use the version in the target repo, not the current/parent. However, using the package.json of the current/parent isn't foolproof either (e.g., the target could have added a dependency that we don't have, one that depends on packages we do depend on but depends on different versions).

The other approach would be to do a fake gx-workspace update. However, that's way more work.

schomatis · 2018-08-31T00:44:14Z

The only difference is that, if the target repo has added a new dependency also present in the current/parent package, we'll use the version in the target repo, not the current/parent. However, using the package.json of the current/parent isn't foolproof either (e.g., the target could have added a dependency that we don't have, one that depends on packages we do depend on but depends on different versions).

Let's have both options: --deps=dependent and --deps=dependency (we can use other terms but let's keep them absolute, not relative to the current/mine/theirs perspective) and recommend the second one (and have it as the default) as the conservative approach (noting that it not foolproof).

Stebalien · 2018-08-31T01:14:45Z

Let's have both options: --deps=dependent and --deps=dependency (we can use other terms but let's keep them absolute, not relative to the current/mine/theirs perspective) and recommend the second one (and have it as the default) as the conservative approach (noting that it not foolproof).

In this case, the other strategy would really be "original" or "replaced" (dependency is could either mean the dependency that I'm replacing/linking or the one I'm linking to).

I'm really confused why you feel so strongly about keeping the terminology absolute. Personally, I find it extremely confusing (depending on the context). When I see an absolute term, I think "oh, they must not mean my current package, otherwise they would have just said that". Specifically, when I see "dependent", my first thought is "which dependent?".

schomatis · 2018-08-31T01:41:35Z

I'm really confused why you feel so strongly about keeping the terminology absolute. Personally, I find it extremely confusing (depending on the context).

Ok, let me rethink that and get back to you.

I think that my main objection is about the code and not the UX. From the point of the user who just runs cd package; gx-go link dependency I think you're right, "current" is the package we just moved to, there's no doubt there. Using "current" in the code starts losing meaning as we're constantly changing perspectives between dependency-dependent packages, and the original ("current") package from where we (the user) started has little to do with the ongoing operation of, for example, fetching the dependency of the the dependency. That's where I want to have a clear model of which package is on top from where we are extracting its dependencies to later convert (fetch) them into other (also) packages. But yes, that part of the code is a different realm from the command the user runs.

Stebalien · 2018-08-31T02:04:41Z

Ah, yes, that's totally reasonable. parent actually seems like a good word in that case (or root).

(This patch changes the current API.) Add `--override-deps` flag to (in the case a of a dependency version mismatch) replace the dependency version of the target package with the one in the current dependent package. (Most of the implementation logic for this feature had to be added to the `post-install` hook which also has a new `--override-deps` flag.) Require the command to be run inside a package. Refactor the link/unlink functions. Give more visibility to the options that allows specifying a dependency to link by its name.

schomatis · 2018-09-03T15:02:47Z

@Stebalien Could you take another look please? Changed the terminology to target/current (using your command description) and renamed the option --override-deps. I've finally had to settle for just using the versions from the current dependent package (and didn't add the variation of taking them from the target package because this command overwrites it -since everything is done in the same global directory- and wanted to avoid changing too much functionality in this first iteration).

schomatis · 2018-09-12T16:17:08Z

@Stebalien

schomatis · 2018-09-23T17:35:54Z

@Stebalien

Stebalien · 2018-09-29T02:20:50Z

Ok, this looks correct. Sorry this took so long, I'm very unsure of how any of this code works so I kept on punting.

schomatis added enhancement in progress labels Aug 29, 2018

schomatis self-assigned this Aug 29, 2018

schomatis mentioned this pull request Aug 29, 2018

Synchronize dependencies with another package and link to vendor directory #48

Closed

schomatis requested a review from Stebalien August 29, 2018 16:23

schomatis changed the title ~~[WIP] link: sync deps with parent package~~ link: sync deps with parent package Aug 29, 2018

schomatis removed the in progress label Aug 29, 2018

Stebalien reviewed Aug 30, 2018

View reviewed changes

schomatis mentioned this pull request Aug 31, 2018

empty .gx/post-install created whyrusleeping/gx#208

Open

schomatis added 3 commits September 3, 2018 11:45

main: refactor loadDep

30fa0b4

rw --fix: install packages if needed

02f19cf

schomatis changed the title ~~link: sync deps with parent package~~ link: override dependency versions with the current dependent package Sep 3, 2018

loadDep: fetch package if the dependency isn't already installed

baccffa

Stebalien approved these changes Sep 29, 2018

View reviewed changes

Stebalien merged commit ad7e5a1 into whyrusleeping:master Sep 29, 2018

schomatis deleted the feat/link/sync-deps branch September 29, 2018 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

link: override dependency versions with the current dependent package #51

link: override dependency versions with the current dependent package #51

schomatis commented Aug 29, 2018 •

edited

Loading

schomatis commented Aug 29, 2018

schomatis commented Aug 29, 2018

Stebalien Aug 29, 2018

Stebalien Aug 29, 2018

schomatis Aug 30, 2018

Stebalien Aug 30, 2018

schomatis Aug 31, 2018

Stebalien Aug 29, 2018

Stebalien Aug 29, 2018

Stebalien Aug 29, 2018

schomatis Aug 30, 2018

Stebalien Aug 30, 2018

Stebalien Aug 29, 2018

schomatis Aug 30, 2018

Stebalien commented Aug 30, 2018

Stebalien commented Aug 30, 2018

schomatis commented Aug 31, 2018

Stebalien commented Aug 31, 2018 •

edited

Loading

schomatis commented Aug 31, 2018

Stebalien commented Aug 31, 2018

schomatis commented Sep 3, 2018

schomatis commented Sep 12, 2018

schomatis commented Sep 23, 2018

Stebalien commented Sep 29, 2018

link: override dependency versions with the current dependent package #51

link: override dependency versions with the current dependent package #51

Conversation

schomatis commented Aug 29, 2018 • edited Loading

schomatis commented Aug 29, 2018

schomatis commented Aug 29, 2018

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

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

Stebalien commented Aug 30, 2018

Stebalien commented Aug 30, 2018

schomatis commented Aug 31, 2018

Stebalien commented Aug 31, 2018 • edited Loading

schomatis commented Aug 31, 2018

Stebalien commented Aug 31, 2018

schomatis commented Sep 3, 2018

schomatis commented Sep 12, 2018

schomatis commented Sep 23, 2018

Stebalien commented Sep 29, 2018

schomatis commented Aug 29, 2018 •

edited

Loading

Stebalien commented Aug 31, 2018 •

edited

Loading