Make sure we don't try to include an empty set #1571

Merged
merged 1 commit into from Sep 26, 2017

Conversation

Projects
None yet
4 participants
Contributor

aleixpol commented Sep 25, 2017

This happens when snapcraft loads the configuration in cleanbuild to
retrieve the name and some other metadata. It's too early to resolve the
parts and they are null outside of the container.


  • 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? < some tests fail but they also failed before as well
Member

kyrofa commented Sep 26, 2017

Missing some tests here, which we can of course help with. Other reviewers: another option for this is to stop parsing the config so completely before spinning up a cleanbuild (this applies to persistent containers as well). I'm slightly concerned that, while this patch works, it'll be easy to introduce similar behavior in the config parser that breaks on other distros in the future. Thoughts?

Collaborator

kalikiana commented Sep 26, 2017

Good catch! I actually tried out snapcraft cleanbuild on Fedora the other day, so good timing! I can reproduce it with a snap that uses a git part.

It occurs to me this sort of works because of DummyRepo.get_packages_for_source_type - all other methods will fail with NotImplementedError(). I feel that method should never be called in the first place...
How about lazily loading the PartsConfig in _config.py? eg. make parts a property and assign it the first time it's called.
If we wanted to go one step further, maybe a slimmer base class for Config that only processes metadata would actually be more future proof? Ideally it would not contain anything that goes beyond reading the YAML.

@kalikiana kalikiana requested review from kalikiana and kyrofa Sep 26, 2017

repo: return a proper value in DummyRepo
The DummyRepo call for get_packages_for_source_type should return
an expected value by the API which is a set.

This happens when snapcraft loads the configuration in cleanbuild to
retrieve the name and some other metadata. It's too early to resolve the
parts and they are null outside of the container.

@sergiusens sergiusens merged commit 898c989 into snapcore:master Sep 26, 2017

1 check passed

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

@sergiusens sergiusens added this to the 2.35 milestone Sep 26, 2017

@sergiusens sergiusens added the bug label Sep 26, 2017

@aleixpol aleixpol deleted the aleixpol:set branch Sep 26, 2017

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