Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vendor libp2p with gx #2256

Merged
merged 30 commits into from
Feb 6, 2016
Merged

vendor libp2p with gx #2256

merged 30 commits into from
Feb 6, 2016

Conversation

whyrusleeping
Copy link
Member

The biggest part of this PR will be making sure everything is still easy to dev with

@rht
Copy link
Contributor

rht commented Jan 29, 2016

(this should take <=~O(1day) the longest)

@daviddias
Copy link
Member

image

@daviddias
Copy link
Member


Seal of Approval :D (//cc @RichardLitt)

@whyrusleeping
Copy link
Member Author

i still need to fix the tests so they know what the hell is going on

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping
Copy link
Member Author

now i'm running into docker issues... id like to copy $GOPATH/src/gx into the build context, but i dont see any easy way to do that. Its looking like i'll have to install gx on the docker image and run an install... which is annoying because i've already done it once during the CI test runs...

cc @lgierth

@ghost
Copy link

ghost commented Feb 5, 2016

@whyrusleeping it's good now

@whyrusleeping
Copy link
Member Author

oh my god look those tests are green o.o

@whyrusleeping
Copy link
Member Author

@RichardLitt can you take a look at the usability here? Maybe help me out making the readme clear on how this works (all anyone should have to do is run make install)

@whyrusleeping
Copy link
Member Author

i'll merge this in the morning, we can always improve the readme in a followup PR

@chriscool
Copy link
Contributor

In the sharness tests some work has been done to remove any dependency on wget and to use curl instead. It would be nice here to use curl as much as possible instead of wget.

@ghost
Copy link

ghost commented Feb 5, 2016

@chriscool wget is being used inside the container. I tried curl first but it doesn't have the retrying options I was looking for (retrying unconditionally).

@rht
Copy link
Contributor

rht commented Feb 5, 2016

(in other situation, ipfs get/ipget was being used in place of curl since the latter doesn't do recursive download (while wget does) )

@RichardLitt
Copy link
Member

@whyrusleeping I opened github.com//pull/2302 to merge into this branch. Is that what you wanted?

RichardLitt and others added 2 commits February 5, 2016 11:08
I did the following:
- Capitalized IPFS wherever it did not explicitely refer to the CLI command
- Moved Dev Dependencies into the Install section
- Added a Table of Contents
- Capitalised and added periods to various sentences
- Capitalised Go
- Removed double empty lines
- Reindented a few sections to make the flow more clear.
- Added a note to the Install section about how to canonically install.

License: MIT
Signed-off-by: Richard Littauer <[email protected]>
@@ -9,10 +9,12 @@ import (

// TestEmbeddedDocs makes sure we don't forget to regenerate after documentation change
func TestEmbeddedDocs(t *testing.T) {
t.Skip("skipping for now, paths are hard")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really familiar with assets/: what is this test supposed to do and why are we now skipping it? Could we get a tracking issue and link to it here so it doesn't fall entirely off the radar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test is testing that the string constants in this file match the files stored in git. Except now those files are stored in gx (and we arent storing the gx deps in git). I can probably add a bit of logic to search for the installed package either locally or globally. Good catch

@hackergrrl
Copy link
Contributor

Nice work @whyrusleeping. It's been cool watching gx go from proof of concept to powering go-ipfs.

Some usability feedback:

  1. make in cmd/ipfs/ doesn't work: gx looks for package.json, which of course is back in the root directory.
  2. make is now very expensive. Every invocation does go get -u gx, go get -u gx-go, and gx install, even when you're up to date. This makes my builds internet dependent, and significantly slower -- adds ~43 seconds. Other folks might have different workflows, but I rely on make for development; I'd be really keen on keeping it fast. Maybe having dummy files we touch on go get and gx install would work well with make?
  3. More makefile stuff: right now only install depends on deps, but I think build targets should rely on deps instead -- and then we can have install rely on build. Does that make sense?

Kudos @RichardLitt for the nice clean-up to the README; it's an entry point for so many people who look at IPFS. 👍

@whyrusleeping
Copy link
Member Author

@noffle good catch on the cmd/ipfs makefile, missed that one. As for make being expensive, I do want to ensure that anyone building ipfs as per the listed build instructions has their deps up to date. Building without the makefile is quite easy: go install ./cmd/ipfs or go build ./cmd/ipfs. We could have a make fast-install or make fast-build type target for your usecase if you don't want to type the go commands.

having install depend on build depend on deps is a good idea. changing that now.

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
@hackergrrl
Copy link
Contributor

As for make being expensive, I do want to ensure that anyone building ipfs as per the listed build instructions has their deps up to date.

Agreed. Ideally we'd have versions of gx and gx-go pinned so we could skip these heavy steps. I'll think about the problem some more, but yeah: it doesn't make sense to hold up this PR because of it.

@hackergrrl
Copy link
Contributor

🐴 LGTM

whyrusleeping added a commit that referenced this pull request Feb 6, 2016
@whyrusleeping whyrusleeping merged commit 5a0b802 into master Feb 6, 2016
@whyrusleeping whyrusleeping deleted the feat/gx-libp2p branch February 6, 2016 00:52
@rht
Copy link
Contributor

rht commented Feb 6, 2016

Somehow missed the go get -u in the make install. This breaks the immutability of the build of go-ipfs of a specific commit (with gx remains as being mutable).

@hackergrrl
Copy link
Contributor

@rht: yeah +1. I'd like to see a means of retrieving a pinned version of gx and gx-go as well. Maybe via ipget or just wgeting the public gateway?

@rht
Copy link
Contributor

rht commented Feb 7, 2016

It is simplest to just bootstrap gx, gx-go in vendor/ (this is the closest thing to hash/version pinning in go anyway, albeit implicit). That way, the deps that are fetched by gx are still pinned by their hashes or versions from package.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants