Skip to content

Conversation

@AsakuraMizu
Copy link
Contributor

We previously used the --cfg doc hack for documentation writing and testing. While effective, it wasn't elegant. This resulted in the issue shown in #25, namely incompatibility with crates like quote.

This PR adds the all optional feature to both crates. It behaves exactly the same as when we used --cfg doc, except for some tweaks to architecture-specific dependency crates to improve compilation.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces the --cfg doc hack with a proper all feature flag for documentation building and testing, resolving compatibility issues with crates like quote. The feature allows all architecture-specific modules to be compiled regardless of the target architecture, enabling comprehensive documentation and testing.

Key changes:

  • Added all feature to both page_table_entry and page_table_multiarch crates
  • Removed architecture-specific dependencies that caused cross-compilation issues (x86_64 crate, moved aarch64-cpu to dev-dependencies)
  • Added conditional compilation guards for architecture-specific assembly code to prevent compilation errors when using the all feature on non-matching architectures

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
page_table_multiarch/Cargo.toml Added all feature and removed doc cfg from target dependencies
page_table_multiarch/src/arch/mod.rs Updated module cfg attributes to use feature = "all" instead of doc
page_table_multiarch/src/arch/x86_64.rs Added conditional compilation guards for x86_64-specific TLB flush assembly
page_table_multiarch/src/arch/riscv.rs Added conditional compilation guards for RISC-V-specific TLB flush assembly and inlined helper function
page_table_multiarch/tests/alloc_tests.rs Added cfg attributes to test functions to enable running with all feature
page_table_entry/Cargo.toml Added all feature, removed x86_64 dependency, moved aarch64-cpu to dev-dependencies
page_table_entry/src/arch/mod.rs Updated module cfg attributes to use feature = "all" instead of doc
page_table_entry/src/arch/x86_64.rs Replaced x86_64 crate dependency by defining PTF flags inline using bitflags macro
page_table_entry/src/arch/aarch64.rs Converted MAIR_VALUE from computed constant to hardcoded value with doctest example
page_table_entry/README.md Removed obsolete import of PageTableFlags from x86_64 crate in example
Cargo.lock Removed x86_64 crate and its dependencies from lock file
.github/workflows/ci.yml Updated CI to use --all-features instead of --cfg doc and removed obsolete flags

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AsakuraMizu
Copy link
Contributor Author

@Azure-stars Your comments all point to the same issue. My intention was to minimize the use of target-specific dependencies because Cargo's Platform specific dependencies feature doesn't support writing things like [target.'cfg(any(target_arch = "x86_64", feature = "all")).dependencies]. I'm also unsure if there's a way to automatically enable an optional dependency on a specific platform.

@AsakuraMizu
Copy link
Contributor Author

AsakuraMizu commented Nov 21, 2025

Well, you see it. I tried to add these platform-specific crates back, but these two x86 crates are complaining.

  • x86_64 doesn't provide an instructions module on other architectures.
  • x86 even doesn't compile!

In this respect, aarch64-cpu does a good job. It exposes the same interface on all architectures, but changes the internal implementation to unimplemented!() on unsupported architectures.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

3 participants