gx: gx-go uw everything #5435

Stebalien · 2018-09-07T00:01:33Z

And add a dummy package that refuses to build unless we're using gx.
Also remove Godeps

kevina · 2018-09-07T00:10:37Z

@Stebalien #4831 has not been commented on in a while. Was there some discussion on this issue via out of band means? Or is this mostly a proof-of-concept?

Stebalien · 2018-09-07T00:34:42Z

For now, this just rewrites in go-ipfs itself (and doesn't even bother rewriting). While a bit annoying, I consider this the lesser of two evils (compared to keeping the files in the rewritten state).

Stebalien · 2018-09-07T00:35:26Z

I picked this up because I got tired of running into random conflicts when rebasing.

kevina · 2018-09-07T00:44:04Z

@Stebalien while I mostly agree, I don't want to rush this p.r. in. I can see a lot of ways this can break things and, at least in my view, this needs some vetting before it lands.

Stebalien · 2018-09-07T00:49:52Z

Of course! CI doesn't even pass yet.

However, I don't want to just sit on our asses as is our usual MO when it comes to hard choices with no good solutions; I submitted this PR to get the ball rolling again.

magik6k

Yes, Please!

Can you separate the non-rewrite/non-extraction changes into separate commits so it's easier to review?

warpfork · 2018-09-07T12:53:21Z

I would be so happy about this being able to land. ❤️

Stebalien · 2018-09-07T17:48:13Z

Can you separate the non-rewrite/non-extraction changes into separate commits so it's easier to review?

yeah, I was just a bit lazy

Stebalien · 2018-09-07T22:12:09Z

Extracted here: #5440

Stebalien · 2018-09-07T22:13:02Z

Can you separate the non-rewrite/non-extraction changes into separate commits so it's easier to review?

~~Actually, the commits should have been separate. But we should do these one at a time anyways.~~

I see what you mean. Fixed.

magik6k · 2018-09-08T08:32:57Z

Yeah, I meant separate commits, not PRs. Will review once ci passes

magik6k · 2018-10-05T09:47:48Z

@Stebalien updates on this?

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

And add a dummy package that refuses to build unless we're using gx. fixes #4831 License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien · 2018-10-18T16:08:58Z

The alternative is to just switch to the lockfile approach. However, we'll then need to modify ipfs/distributions to set some "release" flag (if we still want gx paths in built binaries). Thoughts?

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien · 2018-10-18T16:13:06Z

However, I think we should go with this for now. Regardless, we're going to leave the paths unrewritten so any changes will be small changes to the build system. This is a step in the right direction.

kevina · 2018-10-18T17:27:50Z

About the same time this lands we need to modify gitcop to check that they are no rewritten paths, it will be very easy to screw this up. In particular it is still possible for "gx rewrite --fix" to miss some paths.

kevina · 2018-10-18T17:45:18Z

mk/gx.mk

@@ -2,6 +2,7 @@ gx-path = gx/ipfs/$(shell gx deps find $(1))/$(1)

 gx-deps:
 	gx install --global
+	gx-go rw


I don't think having the build process modify files in place is a very good idea. The in-place rewrite of the deps in go dependencies already causes me grief in that I have to have emacs automatically reread the files, which doesn't always work. With go deps, I can at control when it is done, here it will be done as part of make build. It can also cause other problems, for example if you have unsaved changes.

Maybe we should copy everything to a temp dir and build and rewrite from that.

On the other side, packages on github might not be compatible at the time so you might end up rewriting either way.

@Kubuxu I think you misunderstand by idea was to (at least by default) not rewrite deps in place but put the rewritten files into a temp directory and build that. This way no inplace modifications are necessary.

kevina

I am really sorry for blocking this, but I think this needs some careful though before we push this though. It has the potential to break a lot of things.

In addition I see the current changes breaking one of my workflows.

kevina · 2018-10-18T18:51:52Z

mk/gx.mk

@@ -2,6 +2,7 @@ gx-path = gx/ipfs/$(shell gx deps find $(1))/$(1)

 gx-deps:
 	gx install --global
+	gx-go rw


Rewriting as part of the build process will make my workflow of working with a partly rewritten tree impossible or at least difficult.

A quick fix for this might be to provide an official way to build without automatically calling the gx-deps target.

@kevina Could describe how you work with a partly rewritten tree? I can't think of a good use case for this, I am not saying there isn't a good use case for it, I just can't think of it 🙂

Devs can run `make lock` to switch to using a lockfile. Then-on, `make deps` will use this instead of the `package.json`. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien · 2018-10-19T09:51:27Z

@kevina: I've added support for gx "lockfiles". If you run make lock, make build and make deps will use that instead of rewriting. The non-rewriting workflow will be:

Checkout a local go-ipfs copy.
Once, and only once, run make lock.
From then on, we'll use the lockfile and never rewrite (until the lockfile is deleted).

This way, normal builds (ipfs/distributions) still work but devs don't have to deal with this.

Stebalien · 2018-10-19T09:52:49Z

~~Note: this still installs the deps twice when using lockfiles, unfortunately. Once in $GOPATH/gx/ipfs... and once in .cache/gx. Really, we can probably get rid of the former~~ fixed.

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

kevina · 2018-10-19T12:59:35Z

@Stebalien I will need to test this out later today to familiarize myself with how this will work.

kevina · 2018-10-19T13:09:21Z

@lanzafame I used a partly rewritten tree when working on a go-cid refactor which involved a lot of dependencies that needed to be changed (not just gx updated) and the process also created some bugs that I needed to iterate on (fix go-cid then test if the bug is fixed in one of go-cid deps, repeat). I didn't want to checkout everything, just the dependencies effected by go-cid, so I did a gx-go rewrite --undo <list of go-cid deps>.

@Stebalien how will such a process work now?

Stebalien · 2018-10-19T13:16:17Z

@kevina assuming you're using the lock version, you can just remove the vendor directory and use packages from your gopath directly. Otherwise, you can replace the symlinks in the vendor directory with symlinks to packages in your go path.

kevina · 2018-10-19T13:17:57Z

@Stebalien I will need to try this out myself later today to make sense of it all. Since this is a major change I hope you can wait for me. Thanks.

kevina

See inline comments.

kevina · 2018-10-19T16:08:19Z

mk/gx.mk

@@ -2,8 +2,7 @@ gx-path = gx/ipfs/$(shell gx deps find $(1))/$(1)

 # Rebuild the lockfile iff it exists.
 gx-deps: $(wildcard gx-lock.json)
-	gx install --global
-	if test -e gx-lock.json; then gx-go rw --fix && gx lock-install; else rm -rf vendor && gx-go rw; fi
+	if test -e gx-lock.json; then gx-go rw --undo && gx lock-install; else rm -rf vendor && gx install --global && gx-go rw; fi


@Stebalien

I would move the dependency installation to the lock target. Reinstalling the exact same dependency here is redundant. Also, I may be mistaken, but I think it will cause problems if we do as you suggest and (a) remove the vender directory or (b) replace the symlinks in the vendor directory with symlinks to packages in your go path.

Also removing thevender directory without promoting the user seams dangerous (there might be stuff in there not related to gx) so it seams that it would be a better idea to just abort if the vender directory exists and let the user deal with it. For clean build this should not be a factor so I don't think this will break anything.

I would move the dependency installation to the lock target. Reinstalling the exact same dependency here is redundant.

The lock target just generates the lockfile, nothing more. gx-deps is the "install the gx-deps" target so it's definitely the right place to put this.

Also, I may be mistaken, but I think it will cause problems if we do as you suggest and (a) remove the vender directory or (b) replace the symlinks in the vendor directory with symlinks to packages in your go path.

Replacing the symlink definitely works.

Removing the entire vendor directory doesn't quite work (intentionally) due to the github.com/gxed/go-require-gx import in cmd/ipfs/ipfs.go. However, if you remove that it should work as long as none of our downstream packages have made breaking changes.

Also removing the vender directory without promoting the user seams dangerous (there might be stuff in there not related to gx) so it seams that it would be a better idea to just abort if the vender directory exists and let the user deal with it.

In general, vendor directories are managed by whatever dependency management system is in-place, not manually by the user. I don't expect users will be surprised.

I'm worried that not removing it will lead to even more confusion. If the user does a gx-go uw, they'll end up using the "locked" deps instead of the normal ones.

@Stebalien

The lock target just generates the lockfile, nothing more.

My concern is that gx-deps target is run automatically by the build and install target. I never liked that, and now it may do undesirable things. I would be happier if there was a way to tell the build system not to rerun the gx-deps target on make build or make install for the cases when you are doing something slightly unusual and know what you are doing. Otherwise if this becomes an issue I may end up having to hack the Makefile.

Replacing the symlink definitely works.

So if you replace the symbolic link, running the gx-deps target won't overright the content that is already in there?

In general, vendor directories are managed by whatever dependency management system is in-place, not manually by the user. I don't expect users will be surprised.

Okay, I withdraw my objection.

So if you replace the symbolic link, running the gx-deps target won't overright the content that is already in there?

It should have worked but apparently we need to fix a bug in gx first.

whyrusleeping/gx#215

README.md

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien · 2019-01-24T21:39:43Z

We're probably going to go with go modules instead of this, at this point.

Stebalien requested a review from Kubuxu as a code owner September 7, 2018 00:01

ghost assigned Stebalien Sep 7, 2018

ghost added the status/in-progress In progress label Sep 7, 2018

Stebalien force-pushed the feat/ungx branch from 415aaaf to d30a388 Compare September 7, 2018 00:32

magik6k reviewed Sep 7, 2018

View reviewed changes

Stebalien force-pushed the feat/ungx branch from d30a388 to d461228 Compare September 7, 2018 22:21

magik6k mentioned this pull request Sep 8, 2018

gx: update peerstore #5442

Merged

schomatis mentioned this pull request Sep 17, 2018

Uncertain how to effectively use gx-workspace for go-ipfs ipfs-inactive/gx-workspace#20

Closed

AminArria mentioned this pull request Oct 4, 2018

Can't compile with go-ipfs because of hardcoded gx imports decentraland/content-service#30

Closed

Stebalien added 2 commits October 18, 2018 16:27

gx: unrewrite

4890d59

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

gx: fix build for unrewrite

23c2fca

And add a dummy package that refuses to build unless we're using gx. fixes #4831 License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the feat/ungx branch from d461228 to 23c2fca Compare October 18, 2018 15:30

Stebalien changed the title ~~gx: gx-go rw everything~~ gx: gx-go uw everything Oct 18, 2018

document the make deps req

fb71117

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien requested review from magik6k and Kubuxu and removed request for Kubuxu October 18, 2018 16:13

Stebalien requested a review from djdv October 18, 2018 16:13

kevina reviewed Oct 18, 2018

View reviewed changes

kevina suggested changes Oct 18, 2018

View reviewed changes

add optional lockfile support

c150b44

Devs can run `make lock` to switch to using a lockfile. Then-on, `make deps` will use this instead of the `package.json`. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

don't double-install deps when using the lockfile

7a7bf2c

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien requested a review from kevina October 19, 2018 10:13

Kubuxu approved these changes Oct 19, 2018

View reviewed changes

kevina suggested changes Oct 20, 2018

View reviewed changes

add CI step to check for gx-rewritten paths

85cd479

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the feat/ungx branch from fe9f6d3 to 85cd479 Compare October 22, 2018 17:55

Stebalien mentioned this pull request Oct 24, 2018

pubsub overloading issue filecoin-project/venus#1144

Closed

lanzafame mentioned this pull request Dec 17, 2018

Go module support #5849

Closed

Stebalien closed this Jan 24, 2019

ghost removed the status/in-progress In progress label Jan 24, 2019

anacrolix mentioned this pull request Feb 20, 2019

Migrate to go modules: incompatible (>=v2) packages #5956

Closed

hacdias deleted the feat/ungx branch May 9, 2023 10:55

gx: gx-go uw everything #5435

gx: gx-go uw everything #5435

Conversation

Stebalien commented Sep 7, 2018 • edited Loading

kevina commented Sep 7, 2018

Stebalien commented Sep 7, 2018

Stebalien commented Sep 7, 2018

kevina commented Sep 7, 2018

Stebalien commented Sep 7, 2018

magik6k left a comment

Choose a reason for hiding this comment

warpfork commented Sep 7, 2018

Stebalien commented Sep 7, 2018

Stebalien commented Sep 7, 2018

Stebalien commented Sep 7, 2018 • edited Loading

magik6k commented Sep 8, 2018

magik6k commented Oct 5, 2018

Stebalien commented Oct 18, 2018 • edited Loading

Stebalien commented Oct 18, 2018

kevina commented Oct 18, 2018

kevina Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Kubuxu Oct 18, 2018

Choose a reason for hiding this comment

kevina Oct 18, 2018

Choose a reason for hiding this comment

kevina left a comment • edited Loading

Choose a reason for hiding this comment

kevina Oct 18, 2018

Choose a reason for hiding this comment

kevina Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

lanzafame Oct 19, 2018

Choose a reason for hiding this comment

Stebalien commented Oct 19, 2018

Stebalien commented Oct 19, 2018 • edited Loading

kevina commented Oct 19, 2018

kevina commented Oct 19, 2018 • edited Loading

Stebalien commented Oct 19, 2018

kevina commented Oct 19, 2018

kevina left a comment • edited Loading

Choose a reason for hiding this comment

kevina Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Stebalien Oct 22, 2018

Choose a reason for hiding this comment

kevina Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Stebalien Oct 22, 2018

Choose a reason for hiding this comment

Stebalien Oct 22, 2018

Choose a reason for hiding this comment

Stebalien commented Jan 24, 2019

Stebalien commented Sep 7, 2018 •

edited

Loading

Stebalien commented Sep 7, 2018 •

edited

Loading

Stebalien commented Oct 18, 2018 •

edited

Loading

kevina Oct 18, 2018 •

edited

Loading

kevina left a comment •

edited

Loading

kevina Oct 18, 2018 •

edited

Loading

Stebalien commented Oct 19, 2018 •

edited

Loading

kevina commented Oct 19, 2018 •

edited

Loading

kevina left a comment •

edited

Loading

kevina Oct 19, 2018 •

edited

Loading

kevina Oct 22, 2018 •

edited

Loading