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

Add Conan recipe for VTD simulator #3

Merged
merged 11 commits into from
Feb 3, 2021
Merged

Add Conan recipe for VTD simulator #3

merged 11 commits into from
Feb 3, 2021

Conversation

cassava
Copy link
Contributor

@cassava cassava commented Jan 8, 2021

Previously, we relied on a user-installed instance of VTD and used the environment variable VTD_ROOT to help us find this instance. By providing users a way to create a local Conan package, we achieve a better integration with cloe-launch and the rest of the build system.


This change is Reviewable

@cassava cassava added this to the 0.18.0 milestone Jan 8, 2021
@cassava cassava force-pushed the ben/add-vtd-package branch from 1e90c28 to f6dac19 Compare January 15, 2021 15:27
@cassava cassava self-assigned this Jan 19, 2021
@cassava cassava requested a review from clssn January 19, 2021 13:59
@cassava cassava added dependencies Pull requests that update a dependency file enhancement New feature or request labels Jan 20, 2021
Copy link
Contributor

@clssn clssn left a comment

Choose a reason for hiding this comment

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

I had some trouble installing and using cloe-launch. Let's have a look whether that's in scope of this PR or not.

cli/cloe_launch/exec.py Outdated Show resolved Hide resolved
cli/cloe_launch/exec.py Outdated Show resolved Hide resolved
vendor/vtd/conanfile.py Show resolved Hide resolved
vendor/vtd-api/conanfile.py Show resolved Hide resolved
plugins/vtd/bin/vtd Show resolved Hide resolved
plugins/vtd/bin/vtd Outdated Show resolved Hide resolved
plugins/vtd/bin/vtd Outdated Show resolved Hide resolved
@cassava
Copy link
Contributor Author

cassava commented Jan 25, 2021

@clssn, Neither pipx nor pip can install as editable a package that contains a pyproject.toml file. Delete the file, and everything works fine. This is a problem with PEP 517, supposedly.

See: pypa/pipx#151

@tobifalk tobifalk force-pushed the ben/add-vtd-package branch from 8c6fb99 to 152254c Compare January 26, 2021 10:53
plugins/vtd/bin/vtd Outdated Show resolved Hide resolved
plugins/vtd/bin/vtd Outdated Show resolved Hide resolved
@cassava cassava force-pushed the ben/add-vtd-package branch from f2f8ddd to c1d8c19 Compare February 2, 2021 08:51
@cassava
Copy link
Contributor Author

cassava commented Feb 2, 2021

I resolved all noted issues and rebased the branch.

cassava and others added 3 commits February 3, 2021 14:03
- This also involves rewriting the `vtd` script which is used to
  launch and terminate VTD. This script is now rewritten in Python
  and it creates a VTD sandbox first. This will probably be moved
  to the VTD Conan package itself in due time.
@cassava cassava force-pushed the ben/add-vtd-package branch from c1d8c19 to 449b5c9 Compare February 3, 2021 13:07
This also allows us to run smoketests in the Docker container,
since we no longer require VTD to be available to run the
smoketests.
Copy link
Contributor

@clssn clssn left a comment

Choose a reason for hiding this comment

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

Cancelling obsolete review.

vendor/vtd-api/conanfile.py Outdated Show resolved Hide resolved
vendor/vtd/conanfile.py Outdated Show resolved Hide resolved
plugins/vtd/conanfile.py Show resolved Hide resolved
cli/Makefile Show resolved Hide resolved
Copy link
Contributor

@clssn clssn left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3, 1 of 9 files at r5, 3 of 4 files at r6, 10 of 11 files at r7.
Reviewable status: 14 of 32 files reviewed, 2 unresolved discussions (waiting on @cassava and @clssn)


dist/docker/Dockerfile.archlinux, line 37 at r7 (raw file):

COPY . /cloe
RUN make export-vendor export && \
    make WITH_VTD=0 package-all && \
  • Let's make an issue to use a WITH_VTD Dockerfile ARG defaulting to 0, same for other Dockerfiles

dist/docker/Dockerfile.ubuntu, line 62 at r7 (raw file):

    make WITH_VTD=0 package-all && \
    # Run smoketests.
    export LC_ALL=C.UTF-8 LANG=C.UTF-8 && \
  • Add this to Dockerfile.archlinux

@cassava cassava force-pushed the ben/add-vtd-package branch from 449b5c9 to 1cc63c2 Compare February 3, 2021 16:10
Copy link
Contributor Author

@cassava cassava left a comment

Choose a reason for hiding this comment

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

Reviewable status: 13 of 32 files reviewed, 3 unresolved discussions (waiting on @cassava and @clssn)


cli/Makefile, line 10 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

Solved in the latest revision.

Done.


dist/docker/Dockerfile.archlinux, line 37 at r7 (raw file):

Previously, clssn (Henning Claßen) wrote…
  • Let's make an issue to use a WITH_VTD Dockerfile ARG defaulting to 0, same for other Dockerfiles

Done.


dist/docker/Dockerfile.ubuntu, line 62 at r7 (raw file):

Previously, clssn (Henning Claßen) wrote…
  • Add this to Dockerfile.archlinux

Done.

Copy link
Contributor

@clssn clssn left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r7, 1 of 1 files at r8.
Reviewable status: 15 of 32 files reviewed, 1 unresolved discussion (waiting on @cassava and @clssn)

@tobifalk tobifalk self-requested a review February 3, 2021 16:48
@cassava cassava merged commit 7422e3e into master Feb 3, 2021
@cassava cassava deleted the ben/add-vtd-package branch February 3, 2021 16:55
@clsim clsim mentioned this pull request May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants