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

Force series16 to install dependencies #661

Merged
merged 9 commits into from
Aug 15, 2023
Merged

Force series16 to install dependencies #661

merged 9 commits into from
Aug 15, 2023

Conversation

Hook25
Copy link
Collaborator

@Hook25 Hook25 commented Aug 11, 2023

Description

Right now the series16 snap does not install the dependencies. The reason seems to be that the latest version that support python3.5 does not support reading dependencies from pyproject.

This forces the install of dependencies in the snapcraft.yaml.

Also, in order to detect any funny business in the future with dependencies, this adds a very simple test and metabox scenario that tries to import every dependency.

Resolved issues

Checkbox16 2.9.1 is broken
Metabox coverage of dependencies was lacking

Documentation

This also documents why every step of the install is done in more detail with comments

Tests

This adds a new metabox test. To run it call metabox with the VerifyInstallation tag like this:

metabox --tag VerifyInstallation local-source-16.04.py

To test a custom built snap create a metabox configuration (snap-16.04.py):

configuration = {
    "local": {
        "origin": "classic-snap",
        "checkbox_snap": {"uri": "~/checkbox16.snap"},
        "checkbox_core_snap": {"risk": "edge"},
        "releases": ["xenial"],
    },
}

Build the checkbox16 snap and put it in the location you specified above, then run:

metabox --tag VerifyInstallation snap-16.04.py

There does not seem to be another way to install the dependencies
using the pyproject key. This is due to the fact that there is no
pip/setuptools/other backend that seem to support it on such an old
version of python
Minor: python may not be python3, call python3
@Hook25 Hook25 requested a review from kissiel August 11, 2023 14:28
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole premise is great! I absolutely love it and we NEED this. Having said that this IMHO requires some rework.
All the deps in this test should be handled the same way as there are when building checkbox, so we actually test the thing. This also (heavily) applies to transitive dependencies. And the version locks.

I think embedding this test in a test plan is unnecessary. This can be just a job and I'd use checkbox-cli -run
I also think those import from within a job run by Checkbox is not the way the modules would be used. The jobs don't import Checkbox's logic. They do import checkbox-support's logic.

To test if Checkbox can import modules it should have this need to be tested in the same environment that checkbox would run. We facilitated debugging of similar issues in the past by providing the shell entry point to the snap. It has (or at least should have :) ) the same environment as checkbox-cli would have.

Lastly, does the provided example metabox --tag VerifyInstallation local-source-16.04.py even work? VerifyInstallaiton is not a tag. Is the scenario class name automatically appended to the list of tags the scenario has?

metabox/metabox/metabox-provider/bin/dependency_test.py Outdated Show resolved Hide resolved
metabox/metabox/metabox-provider/units/basic-tps.pxu Outdated Show resolved Hide resolved
metabox/metabox/core/machine.py Show resolved Hide resolved
Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maciek already covered a lot. I'm just the annoying typo guy here. :D

@Hook25 Hook25 requested review from kissiel and pieqq August 14, 2023 13:52
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff!
Thanks for all the improvements!

Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that you had a long discussion with Maciek, so we should be all good now. Thanks!

@pieqq
Copy link
Collaborator

pieqq commented Aug 15, 2023

(we have to fix those chain scenarios...)

@pieqq pieqq merged commit 8d1089f into main Aug 15, 2023
6 of 10 checks passed
@pieqq pieqq deleted the fix_s16_dependencies branch August 15, 2023 09:51
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

3 participants