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

pluginhandler: do not search installdir or stagedir for dependencies #2942

Merged
merged 7 commits into from Mar 4, 2020

Conversation

cjp256
Copy link
Contributor

@cjp256 cjp256 commented Feb 14, 2020

The search paths already include primedir which has all of the primed
and staged files at this point. The installdir and stagedir may
contain files that are not shipped in the final snap, so limit search
to primedir as it should be fully populated.

  • Replace _handle_dependencies() with a simpler _warn_missing_dependencies()
    and _calculate_dependency_paths() called from _handle_elf().

  • Add a couple useful debug prints.

  • Add two tests to cover the case where the dependency is found, and
    another where the dependency is missing. For the latter, the lib is
    removed using stage configuration.

This last test would have previously failed, because the dependency
was found in installdir, and ignored by the missing dependency resolver.

Signed-off-by: Chris Patterson chris.patterson@canonical.com

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

The search paths already include primedir which has all of the primed
and staged files at this point.  The installdir and stagedir may
contain files that are not shipped in the final snap, so limit search
to primedir as it should be fully populated.

- Replace _handle_dependencies() with a simpler _warn_missing_dependencies()
and _calculate_dependency_paths() called from _handle_elf().

- Add a couple useful debug prints.

- Add two tests to cover the case where the dependency is found, and
another where the dependency is missing.  For the latter, the lib is
removed using `stage` configuration.

This last test would have previously failed, because the dependency
was found in installdir, and ignored by the missing dependency resolver.

Signed-off-by: Chris Patterson <chris.patterson@canonical.com>
@cjp256
Copy link
Contributor Author

cjp256 commented Feb 14, 2020

This is the starter fix, but I think we should take it further and eliminate use of _find_directory(). I don't think we should be attempting to (re)-search for dependencies.

If we should in fact be re-searching installdir and stagedir for dependencies that were scuttled along the way, perhaps we should move that logic into MissingDependencyResolver..? I'll hold on this pending further discussion.

@cjp256
Copy link
Contributor Author

cjp256 commented Feb 19, 2020

@sergiusens just to confirm - we did agree that this check should simply be moved into MissingDependencyResolver?

What do you think about a message like:

This part is missing libraries that cannot be satisfied with the currently primed files. If this is not intended, ensure that these files are not filtered out using the filesets, stage, or prime keywords. Missing libraries include:

  • libfoo.so.1, found at parts/foo/install/usr/lib/libfoo.so.1
  • libbar.so.1, found at stage/bar/usr/lib/libfoobar.so.1

And my thought would be to filter those files out from the stage-package list, since they were already built/installed/staged.

@cjp256 cjp256 requested a review from kyrofa February 20, 2020 16:22
Chris Patterson added 2 commits February 21, 2020 15:47
Signed-off-by: Chris Patterson <chris.patterson@canonical.com>
Signed-off-by: Chris Patterson <chris.patterson@canonical.com>
@codecov-io
Copy link

codecov-io commented Feb 21, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@16b73e4). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2942   +/-   ##
=========================================
  Coverage          ?   88.14%           
=========================================
  Files             ?      229           
  Lines             ?    16589           
  Branches          ?     2564           
=========================================
  Hits              ?    14622           
  Misses            ?     1430           
  Partials          ?      537
Impacted Files Coverage Δ
snapcraft/internal/build_providers/_lxd/_lxd.py 21.92% <ø> (ø)
.../internal/build_providers/_multipass/_multipass.py 84.61% <ø> (ø)
...apcraft/internal/build_providers/_base_provider.py 84.84% <100%> (ø)

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 16b73e4...5ddebf7. Read the comment docs.

Chris Patterson and others added 4 commits February 21, 2020 14:17
Signed-off-by: Chris Patterson <chris.patterson@canonical.com>
Signed-off-by: Chris Patterson <chris.patterson@canonical.com>
@sergiusens sergiusens merged commit 80ff193 into master Mar 4, 2020
@sergiusens sergiusens deleted the fix-dep-search branch March 4, 2020 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants