elf: strip the .note.go.buildid to make room for patching elf #1798

Merged
merged 9 commits into from Dec 16, 2017

Conversation

Projects
None yet
2 participants
Collaborator

sergiusens commented Dec 7, 2017

patchelf does not handle the existence of the .note.go.buildid section
so we strip it from the resulting binary as from the perspective of the
snap it is not needed.

A newer patchelf is also required which handles the math required for
the way the headers are layed out when using golang's go.

LP: #1736861

Signed-off-by: Ubuntu sergio.schvezov@canonical.com

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

what about a classic go integration test?

Member

elopio commented Dec 7, 2017

I'm confused about what will happen in the deb in xenial. Will we just warn, and the behaviour will be the same as if we had build the snap with snapcraft 2.35?

Member

elopio commented Dec 7, 2017

With this change, gotty builds successfully.

Collaborator

sergiusens commented Dec 8, 2017

@elopio an integration test is in the works, I was just sharing this with you for you to verify. For the deb we will just add patchelf as a build-snap, it is already in the store on the edge channel.

@sergiusens sergiusens changed the title from go plugin: strip sections that patchelf does not handle to elf: strip the .note.go.buildid to make room for patching elf Dec 11, 2017

sergiusens added some commits Dec 8, 2017

go plugin: strip sections that patchelf does not handle
patchelf does not handle the existence of the .note.go.buildid section
so we strip it from the resulting binary as from the perspective of the
snap it is not needed.

A newer patchelf is also required which handles the math required for
the way the headers are layed out when using golang's go.

LP: #1736861

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

I'm happy if the test passes. This is properly commented, so not pretty but clear.

Should we depend on binutils now that we are using the strip command?

My biggest question is when do you want to release this to stable? I think it needs a lot of testing to build and run classic snaps, so I would prefer to do it on January when I'm back.

Collaborator

sergiusens commented Dec 15, 2017

binutils is a base requirement on every distro, more so in Ubuntu, so I'd say no.
The tests did pass already, I just added that last commit for more versatility.

I do wish to release it to stable as it would solve many vendors problems. We should at least move it to candidate.

elopio approved these changes Dec 16, 2017

all green, let's give it more tries on edge.

@sergiusens sergiusens merged commit fa8b7b1 into snapcore:master Dec 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sergiusens sergiusens deleted the sergiusens:patchelf-go branch Dec 16, 2017

@sergiusens sergiusens added this to the 2.38 milestone Dec 16, 2017

@sergiusens sergiusens added the bug label Dec 16, 2017

@sergiusens sergiusens self-assigned this Dec 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment