Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow specifying the C++ edition #63

Closed
wants to merge 1 commit into from
Closed

Allow specifying the C++ edition #63

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion tracy-client-sys/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,23 @@ fn set_feature_defines(mut c: cc::Build) -> cc::Build {
}

fn build_tracy_client() {
let stdcpp: &str = &std::env::var("TRACY_CLIENT_CPP_EDITION").unwrap_or("c++11".into());
let i = stdcpp.len() - 2;
match &stdcpp[i..] {
"98" | "03" => panic!("Invalid C++ version. C++11 and above only"),
"11" | "14" | "17" | "20" | "2b" => (),
_ => panic!("Invalid C++ version"),
}
if !stdcpp.starts_with("c++") && !stdcpp.starts_with("gnu++") {
eprintln!("Invalid C++ edition")
}

if std::env::var_os("CARGO_FEATURE_ENABLE").is_some() {
set_feature_defines(cc::Build::new())
.file("tracy/TracyClient.cpp")
.warnings(false)
.cpp(true)
.flag_if_supported("-std=c++11")
.flag_if_supported(format!("-std={stdcpp}").as_str())
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, I wonder if it would be better to allow for something like RUST_TRACY_CLIENT_CPPFLAGS instead which is more general and would allow overriding the -std flag without much effort since the latter -std= argument overrides the earlier one.

The logic to get the arguments from CFLAGS/CXXFLAGS seems quite straightforward, at least: https://github.com/rust-lang/cc-rs/blob/17c8858efbeec6094c21973e3d2e9773916b6aa2/src/lib.rs#L2532-L2538

Copy link
Owner

Choose a reason for hiding this comment

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

I ended up making a PR to make this super easy to do: rust-lang/cc-rs#818, but in the meanwhile you could also just copy over the relevant pieces of code from cc-rs into this build script.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewing the linked code, it seems like the PR author's requirements can already be addressed by setting CXXFLAGS?

Copy link
Owner

@nagisa nagisa Jul 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately I don’t believe that’s the case – CXXFLAGS will come earlier on the command line, so the -std=c++11 specified by this crate via flag_if_supported will override the -std arguments passed that other way.

Copy link
Author

Choose a reason for hiding this comment

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

Can you explain how I would do this when rust-lang/cc-rs#818 lands?

Copy link
Owner

Choose a reason for hiding this comment

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

Something along the lines of

let mut builder = set_feature_defines(cc::Build::new());
builder.file(...).warnings(false).cpp(true).flag_if_supported("-std=c++11");
let _ = builder.try_flags_from_environment("TRACY_CLIENT_SYS_CXXFLAGS");
builder....

Then whenever TRACY_CLIENT_SYS_CXXFLAGS=-std=c++14 environment variable is set, the compiler is invoked as

cc ... -std=c++11 -std=c++14 $FILE

which, yes, has the superfluous -std=c++11 flag, but the flexibility to set user-specified flags is now available.

Copy link
Author

Choose a reason for hiding this comment

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

Cool thanks! Could I request a version bump for this when it lands in rust_tracy_client?

.compile("libtracy-client.a");
link_dependencies();
}
Expand Down