-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
std: link to liballoc_system if compiled without the jemalloc feature #37975
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
With this and #37974, I can CROSS compile std using Cargo and the following Cargo.toml: [package]
[dependencies.alloc_system]
path = "$RUST_SRC/src/liballoc_system"
[dependencies.panic_abort]
path = "$RUST_SRC/src/libpanic_abort"
[dependencies.std]
path = "$RUST_SRC/src/libstd"
[profile.dev]
panic = "abort"
[profile.release]
panic = "abort" without any change to rust-src. |
@bors: r+ Awesome progress! |
📌 Commit eb78c71 has been approved by |
⌛ Testing commit eb78c71 with merge 39577d0... |
💔 Test failed - auto-mac-32-opt |
This is blocking Xargo from being able to compile the |
Maybe this is affected by how tests are run? Maybe nondeterministic? I'm not sure :( |
Want to retry? (or let's 🔥jem🔥lloc🔥) |
Hm if it is nondeterministic I'd be wary of landing b/c then it'd just turn into a problem I'd have to debug later... If you try running a few times this surely never reproduces on Linux? |
Note that the nondeterminism is just a wild guess, I don't actually know what the problem is. |
I built this locally for |
This only fails on makefiles build for me; the tests build and run successfully using rustbuild, contingent on stage2 std being build with Here is a patch (a simple one-liner) that fixes the test failures under darwin targets by setting |
eb78c71
to
fa10ef8
Compare
That patch totally makes sense. Thanks for investigating, @HybridEidolon! @bors r=alexcrichton |
📌 Commit fa10ef8 has been approved by |
⌛ Testing commit fa10ef8 with merge 52f48fe... |
💔 Test failed - auto-win-msvc-64-opt |
This isn't going to work on targets that do actually use alloc_system. libstd will have an alloc_system dependency and crates that override the default allocator and are not |
See rust-lang#36963 for rationale closes rust-lang#36963 fixes rust-lang#36488 (indirectly) closes rust-lang#37975
The new logic seems wrong. As @HybridEidolon says, if you apply this patch disabling jemalloc means you can never drop in another allocator crate because std has a hardcoded dependency on liballoc_system. That's a bit sad. Ideally I think it'd be nice to specify an allocator preference in Cargo.toml/cargo command line (used when an allocator must be picked i.e. dylibs, not rlibs), but since that's not possible we can make do with a force_alloc_system feature (which, as before, locks you into a single allocator for stage0). Crucially, this feature is distinct from the jemalloc feature - we don't want someone to be forced into alloc_system just for disabling jemalloc! See #39086 |
I'm going to close this due to inactivity, but it'd be great to land a fix for this still! |
…crichton Make rustbuild force_alloc_system rather than relying on stage0 This 'fixes' jemalloc-less local rebuilds, where we tell cargo that we're actually stage1 (this only fixes the rustbuild path, since I wasn't enthusiastic to dive into the makefiles). There should be one effect from this PR: `--enable-local-rebuild --disable-jemalloc` will successfully build a stage0 std (rather than erroring). Ideally I think it'd be nice to specify an allocator preference in Cargo.toml/cargo command line (used when an allocator must be picked i.e. dylibs, not rlibs), but since that's not possible we can make do with a force_alloc_system feature. Sadly this locks you into a single allocator in the build libstd, making any eventual implementation of #38575 not quite right in this edge case, but clearly not many people exercise the combination of these two flags. This PR is also a substitute for #37975 I think. The crucial difference is that the feature name here is distinct from the jemalloc feature (reused in the previous PR) - we don't want someone to be forced into alloc_system just for disabling jemalloc! Fixes #39054 r? @alexcrichton
fixes #36501
r? @alexcrichton