whyrusleeping / gx-go

gx subtool for golang
MIT License
80 stars 28 forks source link

Synchronize dependencies of a `gx-go link` package with another one to avoid discrepancies #46

Closed schomatis closed 5 years ago

schomatis commented 6 years ago

As discussed in https://github.com/ipfs/go-unixfs/issues/4 (the main scenario being: I'm working on a git-cloned go-unixfs repo and I want to build it against go-unixfs so I gx-go link it but then there are dependency discrepancies), when using gx-go link the dependencies mismatch don't allow to build the go-ipfs repo against the linked dependency.

As a temporary solution I would like to add an (experimental) feature in the gx-go rewrite command to use the dependencies of a parent repository (e.g., go-ipfs) instead of its own when there's a dependency mismatch. This is somewhat of an already implicit (unpremeditated) functionality, as rewrite map collisions are already handled by overwriting themselves (https://github.com/whyrusleeping/gx-go/issues/44).

The tricky part is how to handle the reverse (--undo) process. Since we may have built the rewrite map using dependencies of another package, if that package is modified (some of its dependencies changed/updated), the old rewritten dependencies may not match any of the dependencies in the new rewrite map and they won't get reverted (I'm not sure if the --fix functionality can be of use here).

So, a few options, keeping in mind that this is (hopefully) a temporary solution while bigger design decisions are being made in https://github.com/whyrusleeping/gx/issues/179:

  1. Don't bother with the un-rewrite process, since this feature is intended to be used with the gx-go link command over git-controlled repos, so the rewrite can be reverted with git.

  2. Do the rewrite --undo, but leave the burden on the user to keep track of which version of the parent package it used to sync the dependencies.

  3. (My personal favorite.) Save the rewrite map (something that I think would be desirable in many scenarios besides this one) and revert the imports based on that and do not re-read the linked package dependencies or any other parent package used to sync them. This could be save as a JSON file under the .gx/ directory.

schomatis commented 6 years ago

@Stebalien WDYT?

Stebalien commented 6 years ago

(I'm not sure if the --fix functionality can be of use here)

This is probably the correct way to fix this.

Don't bother with the un-rewrite process, since this feature is intended to be used with the gx-go link command over git-controlled repos, so the rewrite can be reverted with git.

:+1:. On unlink, the user can fix everything themselves (e.g., with gx-go rw --fix).

and revert the imports based on that and do not re-read the linked package dependencies or any other parent package used to sync them. This could be save as a JSON file under the .gx/ directory.

This seems brittle. The "fix" option, as far as I know, just lists the go deps, finds anything that starts with gx/ipfs/..., and then undoes the rewrite. Note: we may also want to auto-install dependencies in this case (I don't believe we currently do).


We actually discussed this with @frrist at lunch today. One thought is to do the linking inside the vendor directory to avoid messing with other packages. That is:

  1. Symlink vendor/gx/ipfs/... to $GOPATH/src/$pkg.
  2. Run gx-go rw normally on the parent directory. As far as I know, this should also rewrite the vendor directory.
schomatis commented 6 years ago

Great, I'll take a close look at --fix then.

Run gx-go rw normally on the parent directory. As far as I know, this should also rewrite the vendor directory.

Could you expand on this please? If the linked package is already in the vendor directory why would I need to rewrite the import paths? Could I just use the canonical unmodified imports? I'm pretty sure I'm missing something.

Stebalien commented 6 years ago

Basically, we need the dependency names to match up. That is, if I'm using gx/ipfs/QmA/my-package for my imports, the linked package also needs to use this path.

The idea here is that we'd temporarily symlink vendor/gx/ipfs/QmA/my-package to github.com/me/my-package and then rewrite all the paths in github.com/me/my-package to match the ones used in the current package.

By doing this in vendor/, we avoid affecting other packages. This is important when using the parent's package.json file as we don't want the following situation:

  1. Work on package A that depends on package C.
  2. gx-go link package C using package A's package.json.
  3. Start working on package B (that depends on package C).
  4. Stuff fails because we're using the dependency tree defined by package A instead of the one defined by package B or C.

Note: this won't allow us to use gx-go link from multiple root packages at the same time. We may want to consider dropping a symlink back to the parent's package.json in .gx/linked-from/ (or something like that).

schomatis commented 6 years ago

Ok, I think I get it now, I agree, although there seems to be two different (but related) problems on the table:

  1. Linked packages sometimes have different shared dependencies than the parent package is being linked to and we need a way to programmatically sync them without bothering the developer.

  2. We're not actually linking one package against another (that's just an useful abstraction), we're linking the package against the global workspace which indirectly affects the parent package we want to link to, with the ugly side effect that any other package that depended on that linked gx/ipfs/QmA/my-package will also be affected by any changes we do to it during the development process (that is, not only the rewritten imports but also any logic or API modifications). That's where the vendor/ solution would be very helpful.

@Stebalien So, if you approve it, I can first submit a PR for the first issue and once I get that working I can attempt your solution for the second problem.

Stebalien commented 6 years ago

The current abstraction is actually linking the dvcs version of the package into the global namespace. The problem here is that fixing 1 invalidates this abstraction.

schomatis commented 6 years ago

The current abstraction is actually linking the dvcs version of the package into the global namespace.

Sorry, I was unclear that I was stating my own (arbitrary) mental model where I think of a package being linked against another package, but I was saying that this is a misconception, it's just my own way of abstracting myself from the real fact that we're actually linking a package against the global namespace.

The problem here is that fixing 1 invalidates this abstraction.

My proposed solution just automated what any developer already did manually, and yes that may break other dependencies on the modified package but not more than what you already gets broken when the developer temporarily runs a gx update to match the dependencies himself. But yes, you're right that it's sloppy to submit a PR with only that fix when we can apply the second solution together and indeed make sure that we won't be breaking other packages. I'll get on it.

schomatis commented 6 years ago

So, this seems to be working in #48 (WIP), as I was thinking about it, Implicit in all this logic is that we are now indeed linking one package against another, that is, have a parent package not use its dependency (child) package obtained through IPFS but instead a git-cloned repo (of the same package with a -potentially- more recent version). We're no longer linking in the global workspace.

I would like to make this the default behavior, given that gx-go link is a development tool, not used for production (I think) and that what has been stated before seems like the correct solution, and I don't think there are many scenarios where a user is going to demand that a repo be linked in the global workspace (as it goes a bit against the Gx ways of doing things, and it could be turn into an optional feature eventually).

The bottom line is that it will make the code much more clear and easy to reason about if I can always assume that when using gx-go link I'm standing in this parent package and I'm linking one of its dependencies, that is, I know the parent-child (package-dependency) relationship and all its related information, instead of just having generic hash that I have to look for in the global workspace (or download it) that may represent a potential dependency of an unknown parent package.

@Stebalien Are you ok with this modification in the API?

schomatis commented 6 years ago

(Assumed the answer was yes and went ahead with the changes.)

schomatis commented 6 years ago

While testing the vendor-type implementation of gx-go link in my own workflow I'm having build problems where the vendored version of a package has a type mismatch with the same package being imported in a different dependency that doesn't have it vendored.

I'm linking go-ipld-format to go-ipfs through its vendor/ directory, but another package, go-merkledag, which also uses the go-ipld-format package (with the same hash, this is not a version mismatch issue) is importing it from the $GOPATH directory (since gx-go link only created the vendor/ directory for the parent package go-ipfs to which the link was being done), this results in errors like

go install -ldflags="-X "github.com/ipfs/go-ipfs/repo".CurrentCommit=93c4f19" ./cmd/ipfs
# github.com/ipfs/go-ipfs/dagutils
dagutils/diff.go:56:24: impossible type assertion:
    *merkledag.ProtoNode does not implement "github.com/ipfs/go-ipfs/vendor/gx/ipfs/QmX5CsuHyVZeTLxgRSYkgLSDQKb9UjE8xnhQzCEJWWWFsC/go-ipld-format".Node (wrong type for Copy method)
        have Copy() "gx/ipfs/QmX5CsuHyVZeTLxgRSYkgLSDQKb9UjE8xnhQzCEJWWWFsC/go-ipld-format".Node
        want Copy() "github.com/ipfs/go-ipfs/vendor/gx/ipfs/QmX5CsuHyVZeTLxgRSYkgLSDQKb9UjE8xnhQzCEJWWWFsC/go-ipld-format".Node

@Stebalien Thoughts?

Stebalien commented 6 years ago

Damn. I we'll have to link everything for this to work. See: https://github.com/golang/proposal/blob/master/design/25719-go15vendor.md#proposal

Note: This will work with https://github.com/whyrusleeping/gx-go/pull/49 as we do link everything with that change.

Stebalien commented 6 years ago

Let's drop the vendoring for now. It'll break some workflows but I'm not sure if there's anything we can do about that.

schomatis commented 6 years ago

Ok, I'll submit a separate PR just for the --sync functionality (that although sub-optimal it's still useful for a new developer who shouldn't be burden with all these implementation problems).

schomatis commented 6 years ago

It'll break some workflows

Why?

Stebalien commented 6 years ago

Why?

Actually, it shouldn't break anything that wasn't already broken. Nevermind.

schomatis commented 6 years ago

So, linking everything is still on the table? That would mean linking all the dependencies in the tree or just the dependencies of the root package?