Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix building libstd without backtrace feature #64444

Merged
merged 3 commits into from
Sep 16, 2019
Merged

fix building libstd without backtrace feature #64444

merged 3 commits into from
Sep 16, 2019

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 13, 2019 •
edited
Loading

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2019
@alexcrichton
Copy link
Member

Thanks! I think this ended up having a bit more #[cfg] than I was hoping for though. Could this perhaps go for an alternative strategy where libstd always depends on the backtrace crate, but only if the standard library's backtrace feature is enabled does it actually enable features of backtrace? If you remove all features from backtrace-the-crate it should basically compile anywhere since it's just a bunch of empty stubs, and that should remove any need for #[cfg] here.

@alexcrichton alexcrichton reopened this Sep 14, 2019
@RalfJung
Copy link
Member Author

@alexcrichton is backtrace without any features Rust-only, or does it still need a C toolchain?
Right now, libstd without features can be built without a C toolchain, which would be nice to preserve I think (once we no longer build a dylib, that makes cross-compilation so much simpler).

@RalfJung
Copy link
Member Author

@alexcrichton I implemented your suggestion. However, for reasons I do not understand, it still builds "backtrace-sys" even when the backtrace feature is not set:

    Updating crates.io index
   Compiling core v0.0.0 (/home/r/src/rust/rustc.2/src/libcore)
   Compiling compiler_builtins v0.1.19
   Compiling libc v0.2.62
   Compiling cc v1.0.45
   Compiling build_helper v0.1.0 (/home/r/src/rust/rustc.2/src/build_helper)
   Compiling cmake v0.1.42
   Compiling unwind v0.0.0 (/home/r/src/rust/rustc.2/src/libunwind)
   Compiling backtrace-sys v0.1.31
   Compiling rustc_tsan v0.0.0 (/home/r/src/rust/rustc.2/src/librustc_tsan)
   Compiling rustc_asan v0.0.0 (/home/r/src/rust/rustc.2/src/librustc_asan)
   Compiling rustc_msan v0.0.0 (/home/r/src/rust/rustc.2/src/librustc_msan)
   Compiling rustc_lsan v0.0.0 (/home/r/src/rust/rustc.2/src/librustc_lsan)
   Compiling std v0.0.0 (/home/r/src/rust/rustc.2/src/libstd)
   Compiling rustc-std-workspace-core v1.99.0 (/home/r/src/rust/rustc.2/src/tools/rustc-std-workspace-core)
   Compiling cfg-if v0.1.8
   Compiling alloc v0.0.0 (/home/r/src/rust/rustc.2/src/liballoc)
   Compiling rustc-demangle v0.1.16
   Compiling panic_abort v0.0.0 (/home/r/src/rust/rustc.2/src/libpanic_abort)
   Compiling backtrace v0.3.37
   Compiling rustc-std-workspace-alloc v1.99.0 (/home/r/src/rust/rustc.2/src/tools/rustc-std-workspace-alloc)
   Compiling panic_unwind v0.0.0 (/home/r/src/rust/rustc.2/src/libpanic_unwind)
   Compiling hashbrown v0.5.0
    Finished release [optimized] target(s) in 26.09s
   Compiling rustc-std-workspace-std v1.99.0 (/home/r/src/rust/rustc.2/src/tools/rustc-std-workspace-std)
   Compiling proc_macro v0.0.0 (/home/r/src/rust/rustc.2/src/libproc_macro)
   Compiling term v0.0.0 (/home/r/src/rust/rustc.2/src/libterm)
   Compiling unicode-width v0.1.6
   Compiling getopts v0.2.21
   Compiling test v0.0.0 (/home/r/src/rust/rustc.2/src/libtest)
    Finished release [optimized] target(s) in 10.36s

@RalfJung
Copy link
Member Author

Ah, I suppose the reason is this in backtrace's Cargo.toml:

rustc-dep-of-std = ["backtrace-sys/rustc-dep-of-std", "cfg-if/rustc-dep-of-std", "core", "compiler_builtins", "libc/rustc-dep-of-std", "rustc-demangle/rustc-dep-of-std"]

@RalfJung RalfJung mentioned this pull request Sep 14, 2019
Closed
@rust-highfive

This comment has been minimized.

# to view
@RalfJung RalfJung mentioned this pull request Sep 16, 2019
Merged
@alexcrichton
Copy link
Member

Looks great! Just one minor nit and otherwise r=me

@RalfJung
Copy link
Member Author

@alexcrichton done (I made log_backtrace default to None instead of trying to reorganize the code).

@bors r=alexcrichton

However, do you have any idea how we could avoid building backtrace-sys when the backtrace feature is not set? Seems like currently, setting the rustc-dep-of-std feature flag of the backtrace crate also enables backtrace-sys.

@bors
Copy link
Contributor

bors commented Sep 16, 2019

📌 Commit 49854c4 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2019
@alexcrichton
Copy link
Member

It may be sort of roundaboutedly possible but it's probably best to just have a feature for the crate itself which, if disabled, causes the build script to return immediately. That way when the backtrace crate really depends on backtrace-sys, it enables something like backtrace-sys/actually-build or something like that.

Centril added a commit to Centril/rust that referenced this pull request Sep 16, 2019
@Centril
Rollup merge of rust-lang#64444 - RalfJung:no-backtrace, r=alexcrichton
2a6a342 
fix building libstd without backtrace feature

Fixes rust-lang#64410

r? @alexcrichton
@Centril Centril mentioned this pull request Sep 16, 2019
Merged
@RalfJung
Copy link
Member Author

So, implicitly enabling "foo" when "foo/feature" is enabled is deliberate? That's quite unfortunate as it is a serious reduction in expressivity; not doing that would be easily worked around by enabling both "foo" and "foo/feature" when needed. Is there a tracking issue for such improvements/changes to the feature mechanism (I guess this one would have to be opt-in or tied to an "edition" or so)?

@RalfJung RalfJung mentioned this pull request Sep 16, 2019
Closed
@est31
Copy link
Member

est31 commented Sep 16, 2019

implicitly enabling "foo" when "foo/feature" is enabled is deliberate? That's quite unfortunate

@RalfJung there is an issue about this: rust-lang/cargo#3494

See also my comment in one of its dupes for a possible syntax that doesn't require a new edition: rust-lang/cargo#7259 (comment)

bors added a commit that referenced this pull request Sep 16, 2019
@bors
Auto merge of #64510 - Centril:rollup-m03zsq8, r=Centril
a44881d 
Rollup of 10 pull requests

Successful merges:

 - #63955 (Make sure interned constants are immutable)
 - #64028 (Stabilize `Vec::new` and `String::new` as `const fn`s)
 - #64119 (ci: ensure all tool maintainers are assignable on issues)
 - #64444 (fix building libstd without backtrace feature)
 - #64446 (Fix build script sanitizer check.)
 - #64451 (when Miri tests are not passing, do not add Miri component)
 - #64467 (Hide diagnostics emitted during --cfg parsing)
 - #64497 (Don't print the "total" `-Ztime-passes` output if `--prints=...` is also given)
 - #64499 (Use `Symbol` in two more functions.)
 - #64504 (use println!() instead of println!(""))

Failed merges:

r? @ghost
@RalfJung RalfJung mentioned this pull request Sep 16, 2019
Closed
@bors bors merged commit 49854c4 into rust-lang:master Sep 16, 2019
@RalfJung RalfJung deleted the no-backtrace branch September 17, 2019 07:28
@RalfJung RalfJung mentioned this pull request Mar 2, 2020
Merged
@eddyb eddyb mentioned this pull request Jul 28, 2022
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

libstd build without backtrace feature broken
5 participants
@RalfJung @alexcrichton @rust-highfive @bors @est31