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

Makefile, bin: Support multiple GOPATH components #2808

Merged
merged 1 commit into from
Jun 9, 2016
Merged

Makefile, bin: Support multiple GOPATH components #2808

merged 1 commit into from
Jun 9, 2016

Conversation

karalabe
Copy link
Contributor

@karalabe karalabe commented Jun 5, 2016

Go's GOPATH environment variable can contain multiple paths, separated by a colon : similar to the PATH env var. In such cases Go uses the first path element to install any packages, however the remaining paths are still used to find dependencies. Because of this it's quite a common setup to have multiple paths listed in GOPATH (e.g. one for development and another for appengine, which has its special installer).

The current Makefile however erroneously assumes that GOPATH contains a single path element, and tries to locate go-ipfs within by appending a string to GOPATH, which is obviously cause an invalid path, and hence fail the path_check rule in the Makefile, even though the required condition is met.

This PR fixes it by moving path handling back into bin/check_go_path, which iterates over all the path components of GOPATH, trying each one of them for the go-ipfs match and only failing if none matches the requirements. Further it was kind of weird to pass in PWD as a parameter to a script, so I moved the first parameter's evaluation into the script as well.

This PR fixes it by splitting up GOPATH along the colon characters, appending the go-ipfs suffix to each component and resolving them individually. The possibly multiple resulting paths are passed to the path verification script to iterate over and check individually.

@ghost
Copy link

ghost commented Jun 5, 2016

I'm generally cool with this change, but we should be aware it introduces a build dependency on realpath. The previous version used make's built-in realpath.

More thoughts?

@karalabe
Copy link
Contributor Author

karalabe commented Jun 5, 2016

Yup, just noticed it myself too. Will try to work around it.

@karalabe
Copy link
Contributor Author

karalabe commented Jun 5, 2016

@lgierth Moved realpath back into the Makefile and sorted out the path splitting, suffixing and real-resolution within make. The external script now just iterates over the individual paths and checks them without doing any operations. PTAL

Edit: The teamcity failure seems to be something unrelated. But please correct me if I'm wrong and actually broke something.

@Kubuxu
Copy link
Member

Kubuxu commented Jun 5, 2016

You are right (just for tracking: #2809)

License: MIT
Signed-off-by: Péter Szilágyi <[email protected]>
@karalabe
Copy link
Contributor Author

karalabe commented Jun 5, 2016

Yup, force pushed to rebuild and all tests are green now. :D

@ghost ghost added the topic/tools Topic tools label Jun 5, 2016
@ghost
Copy link

ghost commented Jun 5, 2016

Cool you pulled it off :) I can't speak for your multiple-gopaths scenario, but I tested with my own symlinked-into-gopath scenario and it still works fine.

LGTM

@ghost ghost added the RFM label Jun 5, 2016
@whyrusleeping
Copy link
Member

@chriscool and @Kubuxu could you CR this? i want to always double check changes that affect the build system

@Kubuxu
Copy link
Member

Kubuxu commented Jun 7, 2016

It looks good to me. I hope there isn't another GOPATH edge case waiting around the coroner 😄.

@whyrusleeping
Copy link
Member

@Kubuxu my sentiments exactly, lol. Its looks good to me, but theres always something tricky going on. its hard to be sure

@chriscool
Copy link
Contributor

It LGTM too. Thanks @karalabe !

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.

4 participants