Allow specifying the C++ edition #63
Conversation
In certain build environments *cough ChromeOS cough* the C++ edition may be hamstrung. For example, chrono may not be available - see https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++-features.md#chrono_banned. To workaround this, allow an environment variable to specify the edition. For example: `TRACY_CLIENT_CPP_EDITION=gnu++14 cargo build` Note that since compilers seem to be resilient to badly formed editions, the implementation itself will check for correctness.
This seems pointlessly fragile. |
There was a problem hiding this comment.
I similarly agree that the additional checks for the -std values aren’t really necessary. The set of valid values will depend on the version of the compiler, and if anybody wanted to circumvent these checks, they could basically set up a wrapper script over their $CXX anyway.
| 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()) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Reviewing the linked code, it seems like the PR author's requirements can already be addressed by setting CXXFLAGS?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you explain how I would do this when rust-lang/cc-rs#818 lands?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool thanks! Could I request a version bump for this when it lands in rust_tracy_client?
|
You can now set the standard you want to use via the |
In certain build environments cough ChromeOS cough the C++ edition may be hamstrung. For example, chrono may not be available - see https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++-features.md#chrono_banned.
To workaround this, allow an environment variable to specify the edition. For example:
TRACY_CLIENT_CPP_EDITION=gnu++14 cargo buildNote that since compilers seem to be resilient to badly formed editions, the implementation itself will check for correctness.