Skip to content

impl ... for Box<CoreType> in alloc should not require #[cfg(not(test))] #135100

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

Open
joshtriplett opened this issue Jan 4, 2025 · 2 comments
Open
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Jan 4, 2025

library/alloc/src/ffi/c_str.rs has:

#[cfg(not(test))]
#[stable(feature = "box_from_c_str", since = "1.17.0")]
impl From<&CStr> for Box<CStr> {

As far as I can tell, #[cfg(not(test))] is a workaround for something like #87534, as evidenced by error messages like:
note: the crate `alloc` is compiled multiple times, possibly with different configurations

alloc is being compiled a second time as part of tests, and the two compilations are conflicting.

  1. This workaround shouldn't be necessary.

  2. This should be much better documented, so it doesn't send people on multi-hour debugging adventures when they try to write new trait impls in alloc.

@joshtriplett joshtriplett added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 4, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 4, 2025
@jieyouxu jieyouxu added A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 4, 2025
@bjorn3
Copy link
Member

bjorn3 commented Jan 4, 2025

This workaround shouldn't be necessary.

This is also necessary for all lang item definitions outside of libcore. For libcore it isn't necessary because all "unit" tests have been moved into a separate crate in library/core/tests (core_tests). For liballoc and libstd some are in library/alloc/tests and library/std/tests, but others are in liballoc and libstd directly because they are testing non-public functions and accessing non-public fields.

By the way the current construction also has other issues. We are depending on cargo compiling the libcore that is a direct dependency with the exact same flags and rustc version as the one libtest from the sysroot depends on to avoid getting an error about duplicate lang items. With rustbuild in practice this works fine, but I haven't been able to exactly match the flags in cg_clif's test suite. As such I had to patch core_tests to be an entirely separate cargo package which doesn't depend on libcore and I can't run liballoc's and libstd's test suite at all.

@bjorn3
Copy link
Member

bjorn3 commented Feb 6, 2025

#136642 helps with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants