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

community/go: upgrade to 1.8 #983

Closed
wants to merge 1 commit into from
Closed

community/go: upgrade to 1.8 #983

wants to merge 1 commit into from

Conversation

errm
Copy link
Contributor

@errm errm commented Mar 6, 2017

default-buildmode-pie.patch has been removed since is addressed upstream
by golang/go@53aec79

set-external-linker.patch adresses golang/go#18243

@errm
Copy link
Contributor Author

errm commented Mar 6, 2017

yuck this is failing and I am looking into why

I think it is somehow related to golang/go#18243

@ncopa
Copy link
Contributor

ncopa commented Mar 7, 2017

The golang/go@53aec79 commit will make default non-pie. I think we want default pie, just like we have gcc defaulting to pie.

@errm
Copy link
Contributor Author

errm commented Mar 7, 2017

golang/go@53aec79 doesn't change the default, it just passes -no-pie to the linker when buildmode is exe.

I can try and get this to build with the patch to change the default buildmode to pie if you think that is better... however I am not sure it is best to deviate from upstream like this unless we have to? If we want pie in stuff we are building its simple enough to set --buildmode=pie.

As I understood this patch was here since go didn't have a way to set -no-pie on the linker, not because we were changing the default behaviour of an upstream tool...

@errm
Copy link
Contributor Author

errm commented Mar 7, 2017

Humm so when trying to build with the default-buildmode-pie.patch

the build fails with

2017/03/07 13:19:05 cannot handle R_TLS_IE (sym sync/atomic.(*Value).Store) when linking internally
go tool dist: FAILED: /home/builder/package/src/go/pkg/tool/linux_amd64/link -o /home/builder/package/src/go/pkg/tool/linux_amd64/go_bootstrap /tmp/go-tool-dist-219041411/cmd/go/_go_.a: exit status 1

so I tried setting GO_EXTLINK_ENABLED=1 to force go to use the external linker for cgo

and then I run into

warning: unable to find runtime/cgo.a
/home/builder/package/src/go/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
/usr/lib/gcc/x86_64-alpine-linux-musl/6.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: unrecognized option '-pagezero_size'
/usr/lib/gcc/x86_64-alpine-linux-musl/6.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: use the --help option for usage information
collect2: error: ld returned 1 exit status

Copy link
Contributor

@fabled fabled left a comment

Choose a reason for hiding this comment

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

There is also comment in abuild:

# The source needs to be installed due to an upstream
# bug (https://github.com/golang/go/issues/2775).
# When this is resolved we can split out the source to a
# go-src sub package.

I believe the above bug is fixed, so the abuild could be simplified based on the comment too. Could this be done at same time as the major version upgrade?

ld.Thearch.TLSIEtoLE = tlsIEtoLE

- ld.Thearch.Linuxdynld = "/lib64/ld-linux-x86-64.so.2"
+ ld.Thearch.Linuxdynld = "/lib/ld-musl-x86_64.so.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you need to patch the ld name here, you likely need to patch it for all supported architectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the path to the dynamic linker right? so I would imagine that we don't need to patch it for other GOOSes ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, just for the other architectures (x86, arm, etc).

@jirutka jirutka added the A-upgrade Upgrades an abuild label Apr 4, 2017
@cblecker
Copy link

Go 1.8.1 has been released. Edge should probably target it, as it contains a number of bug fixes:

@errm
Copy link
Contributor Author

errm commented Apr 24, 2017

Updated to target go 1.8.1

@fabled
Copy link
Contributor

fabled commented Apr 28, 2017

I posted to golang-nuts asking how to make the default buildmode PIE. Seems the build still fails with the pie patch applied.

@fabled
Copy link
Contributor

fabled commented Apr 28, 2017

golang-nuts thread started: https://groups.google.com/forum/#!topic/golang-nuts/gQ6ArCTlPGU

@cblecker
Copy link

It looks like the golang team is working on an alpine builder. I don't know if any of their work could be leveraged here:

@fabled
Copy link
Contributor

fabled commented May 1, 2017

The build can be fixed with:

@@ -72,10 +72,11 @@ build() {
                fi
        done
 
-       ./make.bash --no-clean || return 1
+       ./make.bash
        for os in $_gocross; do
+               local hostarch="$GOARCH"
                for arch in "386" "amd64"; do
-                       GOARCH=$arch GOOS=$os ./make.bash --no-clean || return 1
+                       GOHOSTARCH=$hostarch GOARCH=$arch GOOS=$os ./make.bash
                done
        done

I also asked about this in golang-nuts, see if they can give suggestions how to fix this without removing --no-clean as it slows downs things considerably.

Another things I'd like to fix while at this is splitting the -source package.

@errm
Copy link
Contributor Author

errm commented May 11, 2017

@fabled I have applied your suggested changes...

set-external-linker.patch adresses golang/go#18243
@algitbot
Copy link

Merged in 34c3049 by @ncopa. Thanks for your contribution!

(This pull request has been closed automatically by GitHub PR Closer. If you think that it’s not resolved yet, please add a comment.)

@algitbot algitbot closed this May 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-upgrade Upgrades an abuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants