-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
cfg(doctest) doesn't work as expected #67295
Comments
It seems to be way more complicated than I thought... Apparently, you need the code to be compiled in order to be able to link to it when running tests. However, this first compilation doesn't go through rustdoc at all and therefore, cc @rust-lang/compiler @rust-lang/cargo |
cc @sunjay (in case you saw something I missed?) |
@GuillaumeGomez I'm not familiar with this stuff at all, but I guess we'd have to do something similar to what we do for My original issue just suggested setting |
It needs to be done on cargo then. I had a discussion with @ehuss about this and they told me that it could be a big burden on performances. So To be debated... |
The following is not working, I'm wondering if this is caused by the present bug of if I'm not using cfg(doctest) as intended? mod hawktracer {
cfg_if::cfg_if! {
// Do not mix tracing and tests
if #[cfg(all(feature="tracing", not(test), not(doctest)))] {
pub use rust_hawktracer::*;
} else {
pub use noop_proc_macro::hawktracer;
}
}
} |
@eclipseo That's because of this bug. The cfg currently only works while collecting tests to run. It does not affect compilation in the way you'd expect. |
Ping @rust-lang/cargo |
I don't think we have any additional input to provide. We feel like the cost of rebuilding the library a third time is too high. Unfortunately I don't have any suggestions for alternative approaches. |
Could we make this rebuild a bit smarter and only perform it if we have a |
Unfortunately, `#[cfg(doctest)]` does not seem to work: rust-lang/rust#67295
doctests currently don't work well for conditional features see: rust-lang/rust#67295
Hi @GuillaumeGomez, I just ran into this also — I wanted to add a helper function for my doctests but I couldn't get it to work at all. I was initially encouraged by your Could you perhaps update the article with a warning about this (and a link to this bug)? I think that would be helpful for future readers :-) |
@mgeisler I updated the blog post. |
Thanks a lot! |
I also ran into the need for this and reported it to zulip: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/.23.5Bcfg(test).5D.20within.20doc-tests My usage is more-or-less to have the My specific usage is I would like Keep in mind I am trying to provide an accurate example of a test utility. |
I wanted to only conditionally compile testutils but it's needed in doctests which we can't conditionally compile for: rust-lang/rust#67295
I wanted to only conditionally compile testutils but it's needed in doctests which we can't conditionally compile for: rust-lang/rust#67295
I wanted to only conditionally compile testutils but it's needed in doctests which we can't conditionally compile for: rust-lang/rust#67295
Hi all, I'd like to add that running into this is somewhat confusing if you don't happen to have read the aforementioned blog post. At least, it wasn't clear to me from this section in the rustdoc book that this doesn't work. Maybe this should be clarified if it's not on track to change anytime soon? |
yes it doesn't work as expected
|
This is a cargo limitation unfortunately. |
This commit adds an unstable `ci` feature which reverts back the previous non-compliant opening method because it enables tests to run in CI without needing to painstakingly extract the test binary names from `cargo build --tests --message-format json` and spawn each one in `renderdoccmd capture $NAME`. Besides, this strategy is impossible anyway because doctests compiled on-the-fly by `rustdoc` and do not have external binaries with which to launch with RenderDoc. We also cannot make the `RTLD_NOLOAD` conditional, i.e. `#[cfg(any(test, doctest))]` and `#[cfg(not(any(test, doctest)))]`, due to: rust-lang/rust#67295 As such, this internal crate feature will have to do.
* Fix loading of RenderDoc library It turns out this crate has been loading the RenderDoc API incorrectly: > To do this you'll use your platforms dynamic library functions to see > if the library is open already - e.g. `GetModuleHandle` on Windows, or > or `dlopen` with the `RTLD_NOW | RTLD_NOLOAD` flags if available on > *nix systems. On most platforms you can just search for the module > name - `renderdoc.dll` on Windows, or `librenderdoc.so` on Linux, or > `libVkLayer_GLES_RenderDoc.so` on Android should be sufficient here, > so you don’t need to know the path to where RenderDoc is running from. > This will vary by platform however so consult your platform’s OS > documentation. Then you can use `GetProcAddress` or `dlsym` to fetch > the `RENDERDOC_GetAPI` function using the typedef above. This was reported long ago as an issue, and I'm addressing this now with this commit. It does change the runtime behavior of the library, though: the application no longer attempts to load RenderDoc into memory if not injected from the outside, so `RenderDoc::new()` will now return `Err` when previously it would have succeeded. * Restore integration and doctest support in CI This commit adds an unstable `ci` feature which reverts back the previous non-compliant opening method because it enables tests to run in CI without needing to painstakingly extract the test binary names from `cargo build --tests --message-format json` and spawn each one in `renderdoccmd capture $NAME`. Besides, this strategy is impossible anyway because doctests compiled on-the-fly by `rustdoc` and do not have external binaries with which to launch with RenderDoc. We also cannot make the `RTLD_NOLOAD` conditional, i.e. `#[cfg(any(test, doctest))]` and `#[cfg(not(any(test, doctest)))]`, due to: rust-lang/rust#67295 As such, this internal crate feature will have to do.
In 'cargo test' environments, the SvsmAllocator must not be made the global_allocator. However, the conditional setting of this attribute depends on the 'test' configuration predicate being set in 'cargo test' environments. This seems to not be working properly for documentation tests ([1]), and it's similar for the alternative 'doctest' predicate ([2]). As documentation tests currently don't play an important role for the project, just disable them alltogether until the cargo community has settled on a suitable alternative. [1] rust-lang/rust#45599 [2] rust-lang/rust#67295 Signed-off-by: Nicolai Stange <nstange@suse.de>
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 <carlos.lopez@suse.com>
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 <carlos.lopez@suse.com>
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 <carlos.lopez@suse.com>
Includes a workaround for rust-lang/rust#67295 Global context is only available for wasm, but tests are run natively. Because #[cfg(doctest)] does not work as expected, we include a feature to fill that role. We should eventually switch back over when the fix is merged into cargo.
Run into this today.
I think this a clean and simple solution to this problem. How would it break things? The only case I can think of is:
but it seems convoluted. Also, it could be tested with crater if the worries persist. I think the current behavior is a major shortcoming and needs addressing, as for now there is no way to add doctest specific utils. |
* Refactor context management * Add root context provider * Improve docs * Don't allow global mutation on server side There are some serious pitfalls using global (thread local) state while in SSR time. To avoid major security risks, I've added a strict panic if this is attempted. Also improve api clarity. * Remove panic on global write While this can be a security issue, there are some use cases where it's perfectly viable to utilize thread-local state on the server. This is a case where good documentation should help with the niche pitfall, instead of forbidding it entirely. * Update docs * Provide context to store on creation * Add docs for contexts and srr support * Adjust dispatch method naming * Conditionally allow global context access Only wasm32 targets are allowed global context access. Accessing global context in a multi-threaded environment is unsafe, and not allowed. * Fix examples * Improve docs * Fix unit tests * Rename back to Dispatch::global This does break a lot of code, but is way more descriptive and clear as to what exactly is happening. Since we are still pre-release, breaking changes are expected, and shouldn't inhibit improvements. * Rename with_cx -> new * Rename Dispatch::cx -> Dispatch::context * Fix doc tests Includes a workaround for rust-lang/rust#67295 Global context is only available for wasm, but tests are run natively. Because #[cfg(doctest)] does not work as expected, we include a feature to fill that role. We should eventually switch back over when the fix is merged into cargo. * Update docs * Fix macro Also remove unecessary derives * Improve context docs * Refactor functional hooks slightly * Improve docs
I also ran into this. In my doctests I need functions that are behind cfg(test) for mock/setup purposes. Such fn do not belong in release code. Judging from the number of "me too" comments here, this seems a pretty common problem for people. If another compilation for cfg(doctest) cannot be added, then it really seems that |
First, create empty lib crate.
With
use crate::foo::bar
:With
use foo::bar
:With
use crate::bar
:The text was updated successfully, but these errors were encountered: