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

We should avoid setting GO env variables #3231

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

NickeZ
Copy link
Collaborator

@NickeZ NickeZ commented Mar 10, 2025

It should be enough to temporarily set GOPATH during docker image build to install tools in a shared directory and then add that directory to $PATH.

The defaults on unix are:
GOPATH = ~/go
GOROOT = /usr/local/go
GOMODCACHE = GOPATH[0]/pkg/mod

I will have to test this myself first when I have better internet and can use docker.

@NickeZ NickeZ force-pushed the nickez/remove-gopath branch from f2cbde6 to 90f1960 Compare March 10, 2025 23:15
@@ -14,8 +14,6 @@

SHELL := /bin/bash
WEBROOT := frontends/web
GOPATH ?= $(HOME)/go
Copy link
Collaborator Author

@NickeZ NickeZ Mar 10, 2025

Choose a reason for hiding this comment

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

GOPATH can be any directory on your system. In Unix examples, we will set it to $HOME/go (the default since Go 1.8).

@NickeZ NickeZ force-pushed the nickez/remove-gopath branch 4 times, most recently from 034ef51 to e90f28d Compare March 15, 2025 17:04
@NickeZ NickeZ requested a review from benma March 15, 2025 17:05
@NickeZ
Copy link
Collaborator Author

NickeZ commented Mar 15, 2025

Ready for review, need help publishing container tag :29 to ghcr.io.

@NickeZ NickeZ force-pushed the nickez/remove-gopath branch 5 times, most recently from a0155e4 to c6fd07e Compare March 17, 2025 08:29
It should be enough to temporarily set GOPATH during docker image build
to install tools in a shared directory and then add that directory to
$PATH.

The defaults on linux are:
GOPATH = ~/go
GOROOT = /usr/local/go
GOMODCACHE = GOPATH[0]/pkg/mod
@NickeZ NickeZ force-pushed the nickez/remove-gopath branch from c6fd07e to c3c44ae Compare March 17, 2025 08:31
popd
export PATH="~/go/bin:$PATH"
mkdir -p $(go env GOPATH)/$(dirname $GO_SRC_DIR)
cp -a ../bitbox-wallet-app $(go env GOPATH)/$(dirname $GO_SRC_DIR)
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems wrong, there should be no need to copy the repo somewhere else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is that something from how go used to work? that the go code had to be copied to a certain directory structure?

Copy link
Collaborator Author

@NickeZ NickeZ Mar 17, 2025

Choose a reason for hiding this comment

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

There is this comment earlier:

# Because we need to compile some Go code without modules,
# the source must be placed in a specific directory as expected by Go.
# The path is relative to GOPATH.
GO_SRC_DIR=src/github.com/BitBoxSwiss/bitbox-wallet-app

Copy link
Contributor

Choose a reason for hiding this comment

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

Since Go modules has been introduced, the repo can live anywhere. There is, or was, a catch with gomobile, which isn't compatible with modules:

This forced us to turn off modules, which required the repo to be again in the GOPATH like it was required in the past.

However, GO111MODULE=off was removed in this commit 684e62e#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L31, so maybe even android/iOS can now be built regardless of the location, but I am not sure.

I guess for this PR, you could just leave it as is, as changing this is unrelated to this PR.

Copy link
Contributor

@benma benma left a comment

Choose a reason for hiding this comment

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

utACK

@NickeZ NickeZ merged commit 2e687f3 into BitBoxSwiss:master Mar 17, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants