Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
core: fix symlink resolution in get_core_dynamic_linker #1170
Conversation
|
(I have tested this in an i386 lxd and it worked there) |
|
I'm going to go ahead and hit the "update" button here which will hopefully fix that red X from Travis. |
kyrofa
requested changes
Mar 3, 2017
This looks good to me, although I'd prefer to use a snapcraft-specific exception.
| + seen_paths = set() | ||
| + while True: | ||
| + if dynamic_linker_path in seen_paths: | ||
| + raise EnvironmentError( |
kyrofa
Mar 3, 2017
Member
This won't do anything about the errno in EnvironmentError. Can we please use snapcraft.internal.errors.SnapcraftEnvironmentError instead?
| + if not os.path.islink(dynamic_linker_path): | ||
| + return dynamic_linker_path | ||
| + link_contents = os.readlink(dynamic_linker_path) | ||
| + if link_contents.startswith('/'): |
|
Thanks for this @mwhudson! You are correct that there is no unit test for this, we do however have integration tests and other architectures are run through adt, unfortunately we are not allowed to run subsets of tests of things we know work on other architectures and are restricted to always fail until everything works (it is the ubuntu-dev way) which hides the test results from us. We can however setup some test fixtures on the side to at least run these subsets during development and detach ourselves from the adt results from what would be a deb's tests. Do you mind looking into that @elopio? |
|
Given that your autopkgtests currently succeed on armhf, which has this problem, I suspect they are not sufficient either (none of the tests of classic snaps actually install and run the built snap, afaics?) |
|
the |
|
@sergiusens does @elopio need to look into that before this lands? |
|
ok this does solve the problem too https://code.launchpad.net/~sergiusens/+snap/test-snap (at least for i386) |
sergiusens
merged commit b19e9cf
into
snapcore:master
Mar 7, 2017
added a commit
that referenced
this pull request
Mar 27, 2017
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
mwhudson commentedMar 2, 2017
The existing code did not handle relative symlinks correctly, they were
expanded relative to /snap/core/current, not the directory they resided in.
This is why classic snap builds only worked on amd64 and ppc64el on Launchpad.
LP: #1665165
No test because I don't see how to add one (afaict, get_core_dynamic_linker is currently untested).