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.

Sign up for GitHub

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

Jump to bottom

build: match Go's GOPATH defaults behaviour #4678

Merged
merged 3 commits into from
Mar 25, 2018
Merged

build: match Go's GOPATH defaults behaviour #4678

merged 3 commits into from
Mar 25, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 10, 2018

This came up while setting up a new system, and trying to do Go things without explicitly setting GOPATH.

@ghost ghost added the topic/build Topic build label Feb 10, 2018
@ghost ghost requested a review from Kubuxu February 10, 2018 02:48
@ghost ghost self-assigned this Feb 10, 2018
@ghost ghost added the status/in-progress In progress label Feb 10, 2018
@aerth
Copy link

aerth commented Feb 11, 2018

if go assumed to be installed, the output of the go env GOPATH command should be safe on all platforms.

Kubuxu
Kubuxu reviewed Feb 11, 2018
View reviewed changes
mk/golang.mk Outdated
@@ -1,6 +1,9 @@
# golang utilities
GO_MIN_VERSION = 1.9

# match Go's default GOPATH behaviour
GOPATH ?= $(HOME)/go
Copy link
Member

Choose a reason for hiding this comment

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

I would replace it with export GOPATH := $(shell $(GOCC) env GOPATH)

I am not 100% sure it works but it should.

Kubuxu
Kubuxu reviewed Feb 11, 2018
View reviewed changes
bin/check_go_path Outdated
echo "GOPATH not set, you must have go configured properly to install ipfs"
exit 1
fi
[ -z "$GOPATH" ] && GOPATH="$HOME/go"
Copy link
Member

Choose a reason for hiding this comment

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

Witch changes to Makefile this should run without changes.

Kubuxu
Kubuxu previously requested changes Feb 11, 2018
View reviewed changes
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

As aerth pointed out using go env GOPATH should be best way to go. Can you try it out?

@Kubuxu Kubuxu added the need/author-input Needs input from the original author label Feb 15, 2018
@ghost ghost assigned Kubuxu Mar 9, 2018
Kubuxu
Kubuxu reviewed Mar 9, 2018
View reviewed changes
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

commenting to remove my big red cross

@Kubuxu Kubuxu dismissed their stale review March 9, 2018 09:35

because files were changed

@Kubuxu Kubuxu requested review from Stebalien and magik6k March 9, 2018 09:35
@Kubuxu Kubuxu removed status/in-progress In progress need/author-input Needs input from the original author labels Mar 9, 2018
magik6k
magik6k reviewed Mar 9, 2018
View reviewed changes
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

1 question

bin/check_go_path
echo "GOPATH not set, you must have go configured properly to install ipfs"
exit 1
fi

Copy link
Member

Choose a reason for hiding this comment

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

I'd still keep this check (assuming it didn't break, which it shouldn't) so if something goes wrong it's caught.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree, we are already passing GOPATHs as arguments to this function (as $2, $3 and so on). So this check is IMO not needed.

Copy link
Author

Choose a reason for hiding this comment

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

GOPATH doesn't have to be set anymore. I'm actually not sure why the export in golang.mk doesn't carry over.

Copy link
Member

Choose a reason for hiding this comment

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

It does, I just wanted to make this script be runnable semi stand alone.

@ghost ghost added the status/in-progress In progress label Mar 9, 2018
@Kubuxu Kubuxu requested review from kevina and magik6k March 9, 2018 20:55
@Kubuxu Kubuxu added RFM and removed status/in-progress In progress labels Mar 10, 2018
@Kubuxu Kubuxu added this to the go-ipfs 0.4.15 milestone Mar 13, 2018
whyrusleeping
whyrusleeping reviewed Mar 23, 2018
View reviewed changes
mk/golang.mk Outdated
@@ -1,6 +1,9 @@
# golang utilities
GO_MIN_VERSION = 1.9

# match Go's default GOPATH behaviour
export GOPATH ?= $(shell $(GOCC) env GOPATH)
Copy link
Member

Choose a reason for hiding this comment

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

wait a sec, doesnt this need to go below the definition for GOCC?

Lars Gierth and others added 3 commits March 23, 2018 07:09
@Kubuxu
build: match Go's GOPATH defaults behaviour
ad18295 
License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@Kubuxu
Simplify GOPATH check
8c9725d 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Correct output
67a6e63 
License: MIT
Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
@Kubuxu
Copy link
Member

Kubuxu commented Mar 23, 2018

Fixed

@ghost ghost added the status/in-progress In progress label Mar 23, 2018
@Kubuxu Kubuxu removed the status/in-progress In progress label Mar 23, 2018
@ghost
Copy link
Author

ghost commented Mar 24, 2018

Thanks for shipping this for me @Kubuxu ❤️

@whyrusleeping whyrusleeping merged commit ed82e4e into master Mar 25, 2018
@whyrusleeping whyrusleeping deleted the fix/gopath-default branch March 25, 2018 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevina kevina Awaiting requested review from kevina

Assignees

@Kubuxu Kubuxu

Labels
RFM topic/build Topic build
Projects
None yet
Milestone
go-ipfs 0.4.15
Development

Successfully merging this pull request may close these issues.
5 participants
@aerth @Kubuxu @Stebalien @whyrusleeping @magik6k