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

Enforce osusergo build tag for releases #6862

Merged
merged 3 commits into from May 6, 2019

Conversation

@sapk
Copy link
Member

commented May 6, 2019

Similar to #1690, we need a new tag to use the go version and not the linked cgo version that fail on some distrib. In this case, we are using it to finding the runnning user.

Should fix #6841 and also reported bug on discourse

@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@sapk are you able to reproduce this problem? We need to ensure that the issue is solved by this addition.

@GiteaBot GiteaBot added the lgtm/need 2 label May 6, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 6, 2019

Codecov Report

Merging #6862 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6862      +/-   ##
==========================================
- Coverage   41.18%   41.17%   -0.01%     
==========================================
  Files         425      425              
  Lines       58490    58490              
==========================================
- Hits        24091    24086       -5     
- Misses      31212    31216       +4     
- Partials     3187     3188       +1
Impacted Files Coverage Δ
models/repo_list.go 66.84% <0%> (-1.06%) ⬇️
models/gpg_key.go 55.83% <0%> (-0.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9d538c...d31a319. Read the comment docs.

@lafriks lafriks added the kind/bug label May 6, 2019

@lafriks lafriks added this to the 1.9.0 milestone May 6, 2019

@sapk

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@zeripath I can't reproduce the bug exactly since I don't know (yet) which glib is incompatible with which glib version but the output of the linker go in this direction :

#Without osuserog
root@a4d05b9c2e6c:/gitea# CGO_ENABLED=1 go build -tags 'netgo sqlite sqlite_unlock_notify' -ldflags '-linkmode external -extldflags "-static"' .
# code.gitea.io/gitea
/tmp/go-link-710302265/000011.o: In function `unixDlOpen':
/root/go/pkg/mod/github.com/mattn/go-sqlite3@v1.10.0/sqlite3-binding.c:38461: warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/tmp/go-link-710302265/000015.o: In function `mygetgrouplist':
/workdir/go/src/os/user/getgrouplist_unix.go:16: warning: Using 'getgrouplist' in statically linked applications requires at runtime the shared libraries from theglibc version used for linking
/tmp/go-link-710302265/000014.o: In function `mygetgrgid_r':
/workdir/go/src/os/user/cgo_lookup_unix.go:38: warning: Using 'getgrgid_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/tmp/go-link-710302265/000014.o: In function `mygetgrnam_r':
/workdir/go/src/os/user/cgo_lookup_unix.go:43: warning: Using 'getgrnam_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/tmp/go-link-710302265/000014.o: In function `mygetpwnam_r':
/workdir/go/src/os/user/cgo_lookup_unix.go:33: warning: Using 'getpwnam_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/tmp/go-link-710302265/000014.o: In function `mygetpwuid_r':
/workdir/go/src/os/user/cgo_lookup_unix.go:28: warning: Using 'getpwuid_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
#With osusergo
root@a4d05b9c2e6c:/gitea# CGO_ENABLED=1 go build -tags 'netgo osusergo sqlite sqlite_unlock_notify' -ldflags '-linkmode external -extldflags "-static"' .
# code.gitea.io/gitea
/tmp/go-link-514533019/000011.o: In function `unixDlOpen':
/root/go/pkg/mod/github.com/mattn/go-sqlite3@v1.10.0/sqlite3-binding.c:38461: warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

The released gitea 1.8 binary seems to work on the docker image debian:9.6. I will try to find a tag that doesn't work.

@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

I've been trying to find the broken version too... I think it's libc6 2.26 but I don't know which version of debian has that.

@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

hmm... Is it possible that this might fix the static build problems for Arm7 might be fixed by osusergo too?

@sapk

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

On the docker image 9.6 it is libc6 2.24

@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

ubuntu:bionic has 2.27-3ubuntu1 and works

@sapk

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

For testing, I use :

$ cat test.sh
#!/bin/sh
apt-get update
apt-get upgrade -y
apt-get install -y git wget
dpkg -l libc6
/gitea &
sleep 5s
wget localhost:3000/install
sleep 5s

wget https://github.com/go-gitea/gitea/releases/download/v1.8.0/gitea-1.8.0-linux-amd64 && chmod +x ./gitea-1.8.0-linux-amd64
docker run -ti --rm -v "$(pwd)/gitea-1.8.0-linux-amd64:/gitea" -v "$(pwd)/test.sh:/test.sh" debian:9.6 /test.sh
@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

17.10 works even though its version is 2.26-0ubuntu2.1

Show resolved Hide resolved Makefile
Show resolved Hide resolved Makefile
Show resolved Hide resolved Makefile
@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

so static_build appears to remove a few more warnings too - leaving only dlopen warnings which appears to be because sqlite3 can load shared objects so I think that might be irremovable.

Now the arm7 build from that still doesn't work - it just gives me an illegal instruction.

@sapk

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@zeripath what warning do you got that where remove with static_build ? unixDlOpen was the only I still got using only netgo and osusergo.

@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

oh sorry I'm talking nonsense. I misread your message earlier.

And I've misunderstood how the suggestion was working - the static_build tag must have been internal to that particular commenter's build.

@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@sapk why don't you come to discord and we can chat a bit more without spamming this

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels May 6, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

I've decided to approve even though we can't prove that this will solve the problem.

Unfortunately it looks like this doesn't solve the arm7 problem (at least on pi2)

zeripath and others added some commits May 6, 2019

@lafriks

lafriks approved these changes May 6, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels May 6, 2019

@lafriks lafriks merged commit 650df0b into go-gitea:master May 6, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
@zeripath

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

So this will need to be backported to 1.8 in order for us to fix the issue.

@sapk sapk deleted the sapk-fork:tag-osusergo branch May 6, 2019

@sapk

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@zeripath done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.