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

pluginhandler: library detection instead of injection #2337

Merged
merged 26 commits into from Oct 17, 2018

Conversation

Projects
None yet
3 participants
@sergiusens
Collaborator

sergiusens commented Oct 10, 2018

With bases, the logic to inject binaries from the host will no longer be
at play. Instead, the base snap for the target project will always be
installed and necessity of the library will be determined throught there
instead

If a library is missing a warning will be issued instead of an error
given the existence of the content snap and its potential use.

LP: #1794556

Signed-off-by: Sergio Schvezov 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 tests/unit?

pluginhandler: library detection instead of injection
With bases, the logic to inject binaries from the host will no longer be
at play. Instead, the base snap for the target project will always be
installed and necessity of the library will be determined throught there
instead

If a library is missing a warning will be issued instead of an error
given the existence of the content snap and its potential use.

LP: #1794556

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

@sergiusens sergiusens force-pushed the sergiusens:library-list branch from d19937f to 32a45fd Oct 10, 2018

# We cannot error if we consider the content interface...
logger.warning(
"Files from the build host were use to build this snap "
"but are not part of the snap or base: \n{}".format(formatted_system)

This comment has been minimized.

@kyrofa

kyrofa Oct 10, 2018

Member

I suggest "The {part_name!r} part needs the following libraries that are not included in the snap or base: \n{}\n\nThese dependencies can be satisfied via more stage-packages, more parts, or content sharing.".format(formatted_system)

@sergiusens sergiusens force-pushed the sergiusens:library-list branch from eacacab to 444ee4f Oct 10, 2018

sergiusens added some commits Oct 10, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Oct 15, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@3465f98). Click here to learn what that means.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2337   +/-   ##
=========================================
  Coverage          ?   90.26%           
=========================================
  Files             ?      197           
  Lines             ?    12577           
  Branches          ?     1896           
=========================================
  Hits              ?    11352           
  Misses            ?      846           
  Partials          ?      379
Impacted Files Coverage Δ
snapcraft/__init__.py 100% <ø> (ø)
...pcraft/internal/pluginhandler/_build_attributes.py 100% <ø> (ø)
snapcraft/internal/lifecycle/_runner.py 97.12% <ø> (ø)
snapcraft/internal/common.py 95.13% <ø> (ø)
snapcraft/internal/dirs.py 50% <ø> (ø)
snapcraft/internal/pluginhandler/__init__.py 90.97% <100%> (ø)
snapcraft/project/_project_info.py 92.5% <100%> (ø)
...apcraft/internal/build_providers/_base_provider.py 86.59% <100%> (ø)
snapcraft/internal/elf.py 80.82% <100%> (ø)
snapcraft/project/_sanity_checks.py 100% <100%> (ø)
... and 2 more

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 3465f98...3476743. Read the comment docs.

@sergiusens sergiusens force-pushed the sergiusens:library-list branch 4 times, most recently from 248b38c to 197d9c4 Oct 15, 2018

tests: remove dependency on snapcraft for integration tests
The integration tests import snapcraft into the suite, this causes all
sorts of problems, the most concerning one is test independence.

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

@sergiusens sergiusens force-pushed the sergiusens:library-list branch from 197d9c4 to b2f5b09 Oct 15, 2018

@kyrofa

Seems reasonable. One string tweak, but this seems to be about there. Looking forward to the fake server performance gains.

# This exception is here to help with porting issues with bases. In normal
# executions, when no base is set, the legacy snapcraft will be executed.
raise RuntimeError(
"A base is required for the scenario of snap type {!r}".format(

This comment has been minimized.

@kyrofa

kyrofa Oct 15, 2018

Member

"A base is required for {!r} snaps." conveys the same information in less words.

@kyrofa

kyrofa approved these changes Oct 16, 2018

@sergiusens sergiusens merged commit 48bb4c1 into snapcore:master Oct 17, 2018

2 checks passed

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

@sergiusens sergiusens deleted the sergiusens:library-list branch Oct 17, 2018

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