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

Fix doctests #93

Closed
00xc opened this issue Sep 13, 2023 · 1 comment · Fixed by #94
Closed

Fix doctests #93

00xc opened this issue Sep 13, 2023 · 1 comment · Fixed by #94

Comments

@00xc
Copy link
Member

00xc commented Sep 13, 2023

Doctests are a Rust feature where code tests can be inlined into the documentation itself. That way, documentation examples are also ran as integration tests. For example:

/// Sums two integers.
///
/// ```rust
/// let a = 1;
/// let b = 2;
/// assert_eq!(my_sum(a, b), 3);
/// ```
fn my_sum(a: u32, b: u32) -> u32 {
    ...
}

These tests can be ran via cargo test --target=x86_64-unknown-linux-gnu --doc, which at the moment we do NOT do via our Makefile test target. Using --tests instead of --doc will also work.

Currently, we have some doctests for ImmutAfterInitCell and ImmutAfterInitRef, which are currently failing. Moreover, more doctests will be added while implementing the tasks for #74. Thus, we must:

  • Fix ImmutAfterInit* doctests.
  • Run doctests via make test

Check the doctest documentation here:
https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html

@00xc
Copy link
Member Author

00xc commented Sep 13, 2023

So I see @nicstange already noticed this when pushing 8cdea8e and reached the same conclusion as I have: doctests are broken because cfg(doctest) is not set properly during compilation. This means that we cannot disable our custom global allocator during doctests.

For now I'm able to bypass this by doing:

diff --git a/Makefile b/Makefile
index 127e6b6..0cefcc2 100644
--- a/Makefile
+++ b/Makefile
@@ -18,6 +18,7 @@ all: svsm.bin
 
 test:
        cargo test --target=x86_64-unknown-linux-gnu
+       RUSTFLAGS="--cfg doctest" cargo test --target=x86_64-unknown-linux-gnu --doc
 
 utils/gen_meta: utils/gen_meta.c
        cc -O3 -Wall -o $@ $<
diff --git a/src/mm/alloc.rs b/src/mm/alloc.rs
index 6b3cf63..6e0d4ae 100644
--- a/src/mm/alloc.rs
+++ b/src/mm/alloc.rs
@@ -1282,7 +1282,7 @@ unsafe impl GlobalAlloc for SvsmAllocator {
     }
 }
 
-#[cfg_attr(not(test), global_allocator)]
+#[cfg_attr(not(any(test, doctest)), global_allocator)]
 pub static mut ALLOCATOR: SvsmAllocator = SvsmAllocator::new();
 
 pub fn root_mem_init(pstart: PhysAddr, vstart: VirtAddr, page_count: usize) {

I'm tempted to simply push this as I can think of no other alternative.

00xc added a commit to 00xc/svsm that referenced this issue Sep 13, 2023
Our custom bare-metal allocator (`SvsmAllocator`) does not work during
tests, as they run as regular userspace binaries. Thus, we must not
set it as the global Rust allocator. To do this, we need a
compile-time directive that lets us know we are building doctests and
not the regular binary (`cfg(doctests)`), but sadly it is not
properly set during test compilation(see rust-lang/rust#45599,
rust-lang/rust#67295 and coconut-svsm#93).

In order to bypass this, set the compile time flag ourselves via the
RUSTFLAGS environment variable so that tests work. Also add the new
invocation to the `test` target in the Makefile.

Fixes: coconut-svsm#93
Fixes: 8cdea8e ("Cargo manifest: disable doctests")
Signed-off-by: Carlos López <[email protected]>
00xc added a commit to 00xc/svsm that referenced this issue Sep 13, 2023
Our custom bare-metal allocator (`SvsmAllocator`) does not work during
tests, as they run as regular userspace binaries. Thus, we must not
set it as the global Rust allocator. To do this, we need a
compile-time directive that lets us know we are building doctests and
not the regular binary (`cfg(doctests)`), but sadly it is not
properly set during test compilation(see rust-lang/rust#45599,
rust-lang/rust#67295 and coconut-svsm#93).

In order to bypass this, set the compile time flag ourselves via the
RUSTFLAGS environment variable so that tests work. Also add the new
invocation to the `test` target in the Makefile.

Fixes: coconut-svsm#93
Fixes: 8cdea8e ("Cargo manifest: disable doctests")
Signed-off-by: Carlos López <[email protected]>
00xc added a commit to 00xc/svsm that referenced this issue Sep 13, 2023
Our custom bare-metal allocator (`SvsmAllocator`) does not work during
tests, as they run as regular userspace binaries. Thus, we must not
set it as the global Rust allocator. To do this, we need a
compile-time directive that lets us know we are building doctests and
not the regular binary (`cfg(doctests)`), but sadly it is not
properly set during test compilation (see rust-lang/rust#45599,
rust-lang/rust#67295 and coconut-svsm#93).

In order to bypass this, set the compile time flag ourselves via the
RUSTFLAGS environment variable so that tests work. Also add the new
invocation to the `test` target in the Makefile.

Fixes: coconut-svsm#93
Fixes: 8cdea8e ("Cargo manifest: disable doctests")
Signed-off-by: Carlos López <[email protected]>
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 a pull request may close this issue.

1 participant