Nix packaging #154

lbeierlieb · 2025-04-28T12:12:12Z

This PR adds Nix packaging for smartvmi (including plugins) and all of smartvmi's dependencies that are not yet packaged in nixpkgs.

The Nix-based build locks down all dependencies and prevents online access during the build process, resulting in reproducible builds and avoiding build behavior that is based on the configuration of the system the build runs on.
NixOS is not required to utilize smartvmi built by Nix—installing the Nix package manager, e.g., on Debian, is sufficient.

The PR does not only add new files, there are modifications to existing files as well:

.editorconfig: add indentation settings for nix files
CMake
- search for libvmi and corrosion dependencies with find_package before retrieving them with FetchContent
- only build tests when BUILD_TESTING is set
vmicore/rust_src/.gitignore: allow Cargo.lock file (required for reproducible builds with Nix)
vmicore/Readme.md: mention nix/Readme.md

rageagainsthepc · 2025-04-29T07:45:13Z

I am not convinced that we should add nix packaging to this repository. We would have to maintain this feature despite not being familiar with it. Also, it should be very simple to have a separate repository for packaging purposes. This is what we ourselves are already doing right now. Our packaging lives in another repository as well. It would be great if you could reduce your PR to all the changes that need to be made to the build system in order to make it work with nix packaging, but without the actual nix packaging files.
If you have convincing arguments for adding nix packaging though, I'd be open to hearing them. :) In that case I would at least expect CI integration for nix packaging, so we can verify that we don't break it in the future.

lbeierlieb · 2025-05-02T12:08:12Z

There is no necessity to include the Nix code in this repository. We have worked on the packaging and on the breakpoint implementation in a fork and are now coming around the contributing our changes upstream. We completely understand your concerns and will create a separate repository.

I removed all commits except the necessary CMake changes. The test guarding allows to skip building the tests, which speeds up the build process but is currently also essential to let the Nix sandbox compile the code successfully. When the tests are built, this absolute reference causes an error (vmicore and plugins sources are layed out a bit differently in Nix than in the repo). Let us know if you have any concern with this change.

I removed the change in the how-to-build readme as well. Once we have a separate repo set up, I would like to open another PR adding a note for anyone interested building with Nix.

phip1611 · 2025-05-02T13:10:07Z

I don't think you can drop the Cargo.lock and this:

In theory, yes, you can provide the Cargo.lock externally.. but this is a pain to keep in sync. I'm in favor for a more reproducible build here directly in smartvmi.

lbeierlieb · 2025-05-02T14:38:35Z

Thanks for pointing out my oversight. I added the files back to the PR.

@phip1611 Do you see any other issues in the current state if the Nix code was separate? I now quite like the idea of offering Nix packaging for smartvmi in a repo with the NixOS configuration modules for a KVMI setup (also not yet published).

rageagainsthepc

I have added some comments. I am generally open to discussion, but I would not merge the PR as is because of the deactivated tests.

rageagainsthepc · 2025-05-05T12:14:04Z

plugins/apitracing/CMakeLists.txt

@@ -14,10 +14,12 @@ add_subdirectory(src)
 target_compile_definitions(apitracing-obj PRIVATE BUILD_VERSION="${APITRACING_BUILD_NUMBER}" PLUGIN_VERSION="${APITRACING_VERSION}")
 target_compile_options(apitracing-obj PUBLIC -Wunused -Wunreachable-code -Wall -Wextra -Wpedantic)

-include(CTest)
-add_subdirectory(test)
+if (BUILD_TESTING)


I would consider this bad practice actually. Top level CMakeLists.txt are meant for developers. If you want to simply build, you should use the corresponding cmake file in the src/ directory. This avoids having to put everything behind options and prevents building with unnecessary flags like -Wpedantic, for example. This talk outlines the philosophy we have adopted: https://youtu.be/mrSwJBJ-0z8?feature=shared&t=2498
It might be a bit counterintuitive at first glance, but we think it makes a lot of sense.

Also, these changes will deactivate test execution in our CI if I am not mistaken. It does not look like you have configured this option for the CI.

Thanks for the link to the talk, the concept does make a lot of sense.
I dropped the commit introducing the BUILD_TESTING checks. For context on the Nix side: Building the CMakeLists.txt in the src folder is a bit complicated due to the reference to rust_src, so I chose to include the google-test dependency and compile the tests as well for now (cyberus-technology/vmi-nix#4).

rageagainsthepc · 2025-05-05T12:23:47Z

vmicore/rust_src/.gitignore

@@ -1,2 +1,4 @@
 /target/
-Cargo.lock
+# For reproducible builds with Nix, we must check the file in, although it is a


Seems like in the latest version of the Cargo book this does not seem like the go to for libraries anymore. https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html Nowadays they just say "When in doubt, check Cargo.lock into the version control system". ;) Simply delete all of this instead of commenting it out, please.

I see! I removed the comments in a fixup commit.

rageagainsthepc · 2025-05-05T12:26:39Z

vmicore/rust_src/Cargo.toml

@@ -13,7 +13,7 @@ prost-types = "0.13.3"
 tokio = { version = "1.41", features = ["macros", "rt-multi-thread", "net"] }
 tokio-stream = "0.1.16"
 hyper-util = "0.1.10"
-cxx = "1.0"
+cxx = "=1.0.129" # must match version of the 'cxxbridge-cmd' build dependency


Shouldn't this be ensured by the Cargo.lock file you have just checked in?

The match is currently ensured, yes. The explicit version and comment are supposed to prevent the situation where somebody updates the Cargo.lock at a time where cxx's and cxxbridge-cmd's versions on crates.io are not in sync.
Updating the versions in Cargo.toml is a bit more manual effort but ensures the tool and library versions are always compatible.
If you still have any concerns or alternative suggestions, please let me know.

rageagainsthepc · 2025-05-05T12:37:23Z

vmicore/src/lib/CMakeLists.txt

-FetchContent_MakeAvailable(libvmi)
-target_include_directories(vmicore-lib BEFORE PUBLIC ${libvmi_SOURCE_DIR})
-target_link_libraries(vmicore-lib PUBLIC vmi_shared)
+find_package(libvmi CONFIG)


nit: Maybe it makes sense here to test for the correct libvmi library? CMake has CheckFunctionExists for that. It would probably be a good idea to check for vmi_mmap_guest_2() and vmi_read_w_str() since these functions do not exist in upstream libvmi and would therefore cause the build to fail.

The documentation of CheckFunctionExists recommends the use of CheckSymbolExists, which I could not get to function properly in the Nix sandbox. Instead, I included a compile check with a minimal program. The check fails if libvmi.h does not offer the functions from your fork.
Please let me know if you are happy with this approach as well.

I think the problematic parts of CheckFunctionExists do not apply to our code right? So it should be fine to use this function if it works.
The compile check does seem rather strange to me. :-D
@rageagainsthepc

I would have preferred CheckSymbolExists but I am also fine with CheckSourceCompiles.

rageagainsthepc · 2025-05-05T13:07:11Z

When the tests are built, this absolute reference causes an error (vmicore and plugins sources are layed out a bit differently in Nix than in the repo)

I believe this is actually a mistake on our side. Plugins should not reference internal vmicore code. #155

lbeierlieb · 2025-05-12T15:12:30Z

I believe this is actually a mistake on our side. Plugins should not reference internal vmicore code. #155

Thanks for refactoring so quickly! I rebased the PR commits to the newest upstream head and created a repository for the smartvmi Nix code vmi-nix. The repo allows me to verify that any changes to commits in this PR still allow building smartvmi with Nix externally.
Now, I can get to your requested changes next. Thanks!

the default behavior (retrieving libvmi from github:GDATASoftwareAG/libvmi) is not changed, but this change allows vcpkg or Nix to supply libvmi instead.

the default behavior (retrieving Corrosion from github:corrosion-rs/corrosion) is not changed, but this change allows vcpkg or Nix to supply Corrosion instead.

This change is necessary to enable offline compilation of the cxxbridge-cmd with Cargo. The offline compilation is a requirement to build in the Nix sandbox.

previously, the Nix packaging from this repository assumed that the CMakeLists.txt are modified in such a way that the tests are not built by default. GDATASoftwareAG/smartvmi#154 (comment) requested to remove the behavior. The suggestion to build the CMakeLists.txt from the src folder instead of the top level is great, but after a few tries I did not find the correct derivation settings (e.g., by setting cmakeDir) and chose to just build the tests as well. This commit switches the smartvmi source to a commit without the CMakeLists.txt changes and adds gtest to both vmicore and the plugins (required dependency of the test code).

rageagainsthepc

LGTM. Formatting in CMake files is a bit inconsistent, but since we have no linter for that in the CI... 🤷
I would be great if the fixup commit could be melded into the original commit before merging.

To enable reproducible builds (with Nix), Cargo.lock needs to be tracked in version control.

The fetchContent way of retrieving libvmi specifies the exact branch to retrieve; with find_package, it is possible that an incorrect version of libvmi is passed from vcpkg/nix/etc. This commit adds a check that ensure the functions GDATA added to their libvmi fork are present. The build would fail later, but this explicit error clarifies the issue.

lbeierlieb · 2025-05-23T07:28:38Z

I squashed the fixup commit into the original one.

Thank you for taking the time for proper review and providing constructive feedback!

previously, the official gdata smartvmi repo (https://github.com/GDATASoftwareAG/smartvmi) did not contain the changes necessary to build smartvmi with Nix (without big workarounds). Now, the changes are merged (GDATASoftwareAG/smartvmi#154) and we can use the sources for the Nix build!

lbeierlieb requested review from rageagainsthepc, K-Mayer and cakeless as code owners April 28, 2025 12:12

phip1611 approved these changes Apr 28, 2025

View reviewed changes

lbeierlieb force-pushed the nix-packaging branch from eb5c0cc to e8d4595 Compare May 2, 2025 11:42

rageagainsthepc suggested changes May 5, 2025

View reviewed changes

lbeierlieb force-pushed the nix-packaging branch from 6fa12be to 30f3c7e Compare May 12, 2025 14:58

lbeierlieb added 3 commits May 19, 2025 11:48

cmake: allow libvmi to be provided externally

fb0be78

the default behavior (retrieving libvmi from github:GDATASoftwareAG/libvmi) is not changed, but this change allows vcpkg or Nix to supply libvmi instead.

cmake: allow corrosion to be provided externally

531bad6

the default behavior (retrieving Corrosion from github:corrosion-rs/corrosion) is not changed, but this change allows vcpkg or Nix to supply Corrosion instead.

cargo: pin cxx version with patch-level accuracy

02820a2

This change is necessary to enable offline compilation of the cxxbridge-cmd with Cargo. The offline compilation is a requirement to build in the Nix sandbox.

lbeierlieb mentioned this pull request May 19, 2025

smartvmi: add gtest dependency to build test successfully cyberus-technology/vmi-nix#4

Merged

lbeierlieb force-pushed the nix-packaging branch from 30f3c7e to 7a92acc Compare May 19, 2025 15:40

lbeierlieb requested a review from rageagainsthepc May 19, 2025 16:05

lbeierlieb mentioned this pull request May 20, 2025

Nix packaging lbeierlieb/smartvmi#1

Closed

rageagainsthepc approved these changes May 22, 2025

View reviewed changes

lbeierlieb added 2 commits May 23, 2025 09:21

cargo: track lock file

e097cdd

To enable reproducible builds (with Nix), Cargo.lock needs to be tracked in version control.

lbeierlieb force-pushed the nix-packaging branch from 7a92acc to 69ccd62 Compare May 23, 2025 07:22

K-Mayer approved these changes May 26, 2025

View reviewed changes

K-Mayer merged commit 9c1e79f into GDATASoftwareAG:main Jun 2, 2025
4 checks passed

lbeierlieb mentioned this pull request Jun 5, 2025

flake: use gdata repo for smartvmi sources cyberus-technology/vmi-nix#5

Merged

Nix packaging #154

Nix packaging #154

Uh oh!

Conversation

lbeierlieb commented Apr 28, 2025

Uh oh!

rageagainsthepc commented Apr 29, 2025

Uh oh!

lbeierlieb commented May 2, 2025

Uh oh!

phip1611 commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbeierlieb commented May 2, 2025

Uh oh!

rageagainsthepc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rageagainsthepc commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbeierlieb commented May 12, 2025

Uh oh!

rageagainsthepc left a comment

Choose a reason for hiding this comment

Uh oh!

lbeierlieb commented May 23, 2025

Uh oh!

Uh oh!

Uh oh!

phip1611 commented May 2, 2025 •

edited

Loading

rageagainsthepc commented May 5, 2025 •

edited

Loading