Skip to content

Search harder for INCLUDEOS_VMRUNNER and INCLUDEOS_CHAINLOADER paths#37

Merged
alfreb merged 2 commits intoincludeos:masterfrom
MagnusS:no-env
Sep 18, 2024
Merged

Search harder for INCLUDEOS_VMRUNNER and INCLUDEOS_CHAINLOADER paths#37
alfreb merged 2 commits intoincludeos:masterfrom
MagnusS:no-env

Conversation

@MagnusS
Copy link
Copy Markdown
Member

@MagnusS MagnusS commented Sep 16, 2024

Now that vmrunner is a python package we can use the metadata to find the script location. If INCLUDEOS_VMRUNNER is not set, this adds support for finding the location automatically.

If vmrunner is installed with nix, the chainloader will be in a path listed in the propagatedBuildInputs environment variable. This adds support for searching for ./bin/chainloader in this path if INCLUDEOS_CHAINLOADER is not set.

With these changes vmrunner can be used from a nix shell without having to set the environment variables.

Tested with includeos' ./test.sh to verify that the tests still work with this change (without env variables)

Now that vmrunner is a python package we can use the metadata to find
the script location. If INCLUDEOS_VMRUNNER is not set, this adds support
for finding the location automatically.

If vmrunner is installed with nix, the chainloader will be in a path
listed in the propagatedBuildInputs environment variable. If
INCLUDEOS_CHAINLOADER is not set, search for `/bin/chainloader` in this
path to attempt to find it.
@MagnusS
Copy link
Copy Markdown
Member Author

MagnusS commented Sep 16, 2024

This update also accidentally fixed loading the default config, which requires an existing network bridge or the boot command will fail. I added another commit to remove networking from the default config -- otherwise ./boot [unikernel] will not work from inside nix unless a bridge has been set up in advance.

@alfreb
Copy link
Copy Markdown
Contributor

alfreb commented Sep 17, 2024

Nice - so with these changes, the tests which depend on network devices etc. will have to provide vm.json's to accomodate. I think that makes a lot of sense actually, but will it require a lot of cleaning up in the integration tests?

@MagnusS
Copy link
Copy Markdown
Member Author

MagnusS commented Sep 17, 2024

The path was wrong -- it tried to load it from vmrunner/vmrunner/, so I think load_config has just returned {} for some time. It seems to only give an error if the file exists and the parsing fails, if the file doesn't exist it returns an empty config: https://github.com/includeos/vmrunner/blob/master/vmrunner/vmrunner.py#L1194

Tests are green - I think all of them already configure the network in vm.json

Copy link
Copy Markdown
Contributor

@alfreb alfreb left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense now, thanks!

Comment thread vmrunner/vmrunner.py
if INCLUDEOS_VMRUNNER is None:
from importlib.metadata import files
for p_ in files('vmrunner'):
if '__init__.py' in str(p_):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aha, that's the trick.

Comment thread vmrunner/vmrunner.py
if os.path.isfile(chainloader_candidate):
chainloader = c_path + "/bin"
break

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok makes sense.

@alfreb alfreb merged commit 3ed66bb into includeos:master Sep 18, 2024
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.

2 participants