Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
libraries: exclude the full set of libc6 #1632
Conversation
sergiusens
added this to the 2.35 milestone
Oct 18, 2017
sergiusens
added
the
bug
label
Oct 18, 2017
sergiusens
self-assigned this
Oct 18, 2017
|
I like this, but it seems like a bit of a half measure. In an ideal scenario, wouldn't we want to do this for every package included in the core snap? A list that, of course, we do not have. |
| - return frozenset(['libc.so.6']) | ||
| + logger.debug('Only excluding libc libraries from the release') | ||
| + libc6_libs = [os.path.basename(l) | ||
| + for l in repo.Repo.get_package_libraries('libc6')] |
kyrofa
Oct 18, 2017
Member
get_package_libraries definitely includes more than just libs. Will that cause issues, here?
|
On mié, oct 18, 2017 at 5:51 PM, Kyle Fazzari ***@***.***> wrote:
I like this, but it seems like a bit of a half measure. In an ideal
scenario, wouldn't we want to do this for every package included in
the core snap? A list that, of course, we do not have.
This is one step in the correct direction, we never want half of libc6
consumed into the snap. The correct measure is to disable this
functionality completely and never bring libraries in from the host.
…
|
|
On mié, oct 18, 2017 at 5:54 PM, Kyle Fazzari ***@***.***> wrote:
@kyrofa commented on this pull request.
In snapcraft/internal/libraries.py:
> @@ -73,9 +73,10 @@ def _get_system_libs():
lib_path = None
if not lib_path or not os.path.exists(lib_path):
- logger.debug('No libraries to exclude from this release')
- # Always exclude libc.so.6
- return frozenset(['libc.so.6'])
+ logger.debug('Only excluding libc libraries from the
release')
+ libc6_libs = [os.path.basename(l)
+ for l in
repo.Repo.get_package_libraries('libc6')]
get_package_libraries definitely includes more than just libs. Will
that cause issues, here?
It should really not, but what did it bring? You use this logic in dump
and copy already for similar reasons I believe.
…
|
|
For the record, hitting the timeout again here |
|
Ah ha, once I removed the |
|
@kyrofa What do you mean when you say "this error" here? Would it be worth linking a bug in a comment here? Or just a comment pointing out the problem? |
kalikiana
approved these changes
Oct 23, 2017
Nice. So simple I got a bit suspicious at first :-D
|
#1635 was the other problem which caused initial suspicions on this PR. |
kyrofa
approved these changes
Oct 23, 2017
This is A) necessary and B) works, so +1 from me! The get_package_libraries method probably needs a refactor, but it causes no harm here.
sergiusens commentedOct 18, 2017
•
Edited 1 time
-
sergiusens
Oct 18, 2017
Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com
./runtests.sh static?./runtests.sh unit?