rust-vmm / rust-vmm-container

Container with all dependencies required for running rust-vmm crates integration tests.
Apache License 2.0
65 stars 32 forks source link

Allow libgpiod to be installed and bindgen to be run #67

Closed vireshk closed 1 year ago

vireshk commented 1 year ago

These package are required to be pre-installed for the vhost-device [1] workspace's gpio crate. This pull reuqest adds support to install them.

This is required to migrate vhost-device to the mainline version of libgpiod [2].

[1] https://github.com/rust-vmm/vhost-device [2] https://github.com/rust-vmm/vhost-device/pull/279

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

vireshk commented 1 year ago

@andreeaflorescu @gsserge

I did similar work almost a year back and we got the patch [1] merged and then reverted as I changed my WIP libgpiod changes to keep pre-generated bindings in the package itself for time being.

Finally, I got the rust bindings merged in the upstream libgpiod package [2].

The maintainer didn't like the idea of adding pre-generated bindings in there and I am back to the original problem. Apart from that, we also need the userspace tool, libgpiod, to be available / installed, in order to test vhost-device going forward.

And so this pull request.

[1] https://github.com/rust-vmm/rust-vmm-container/pull/56 [2] https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/

andreeaflorescu commented 1 year ago

@vireshk did you build a container from this recipe and tested that it works with the PR from vhost?

vireshk commented 1 year ago

@vireshk did you build a container from this recipe and tested that it works with the PR from vhost?

I did test the vhost-device (not vhost) pull request (https://github.com/rust-vmm/vhost-device/pull/279) with the container built from this.

vireshk commented 1 year ago

@vireshk did you build a container from this recipe and tested that it works with the PR from vhost?

I did test the vhost-device (not vhost) pull request (rust-vmm/vhost-device#279) with the container built from this.

Though I did that few weeks back, let me try that again to make sure I didn't miss anything before you merge this. I will update the status here once I am done.

andreeaflorescu commented 1 year ago

@vireshk I ran a build on aarch64 and the container build is successful. I didn't run the container though.

vireshk commented 1 year ago

@vireshk I ran a build on aarch64 and the container build is successful. I didn't run the container though.

I need to update the request slightly though as earlier I did a cargo build for vhost-device but didn't run rust-vmm-ci/test_run.py and with cargo test I found an issue now. I just need to provide a more standard install path with --prefix=/usr for the installed library.

I am building the container image again and will push out the change soon.

vireshk commented 1 year ago

@andreeaflorescu I am facing some issues with:

pytest rust-vmm-ci/integration_tests/test_coverage.py

Running everything else from test_run.py has no issues now, but this fails. I guess it has something to do with this:

Command 'CARGO_TARGET_DIR=/vhost-device/kcov_build cargo kcov --all --output /vhost-device/kcov_output -- --exclude-region='mod tests {' --exclude-pattern=${CARGO_HOME:-$HOME/.cargo/},usr/lib/,lib/ --verify'

What does exclude-pattern mean here ? Is it skpping the installed library ?

vireshk commented 1 year ago

The errors are like:

/root/.cargo/git/checkouts/libgpiod-0cd4e7aac6d9745a/27b28ba/bindings/rust/libgpiod/src/line_request.rs:31: undefined reference togpiod_line_request_get_num_lines'`

I think the libraries aren't getting linked for some reason here.

vireshk commented 1 year ago

Other than that, this pull request is ready from container's point of view.

vireshk commented 1 year ago

What does exclude-pattern mean here ? Is it skpping the installed library ?

I tried removing the exclude patterns for libs and the errors are still around. Not sure what else is required to make the linking work.

andreeaflorescu commented 1 year ago

The exclude-pattern is just excluding the files from the coverage report, not from linking. That should not be the problem. I think the problem is with the installation of kcov itself. I am doing some unrelated changes in the Docker image (i.e. updating to a newer Ubuntu version) and I am also hitting some problems. Before we can merge this we need to make sure that the container image still works with the CI becasue otherwise the tests will fail across all repositories.

andreeaflorescu commented 1 year ago

I ran the tests locally on vhost-device with the new container image and the coverage test fails with:

          /root/.cargo/git/checkouts/libgpiod-0cd4e7aac6d9745a/27b28ba/bindings/rust/libgpiod/src/line_config.rs:103: undefined reference to `gpiod_line_config_get_offsets'
          collect2: error: ld returned 1 exit status

  = note: some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
  = note: use the `-l` flag to specify native libraries to link
  = note: use the `cargo:rustc-link-lib` directive to specify the native libraries to link with Cargo (see https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorustc-link-libkindname)

error: could not compile `vhost-device-gpio` due to previous error

It looks like it cannot find the library. I think the vhost-device code might need to be updated or maybe the coverage test needs to be updated to take into consideration the linked library. I will try to run the coverage test on other crates as well to see if there is any problem.

andreeaflorescu commented 1 year ago

Tested this with other code bases and there is no problem, so I will approve the PR.