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.

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=c++17] Builder flag_if_supported/flag ignored #1304

Closed
jwinarske opened this issue Jun 25, 2023 · 8 comments
Closed

[std=c++17] Builder flag_if_supported/flag ignored #1304

jwinarske opened this issue Jun 25, 2023 · 8 comments

Comments

@jwinarske
Copy link

Describe the bug
Using b.flag_if_supported("-std=c++17") has no impact, and the log output will still use -std=c++14. Same for b.flag("-std=c++17")

Snippet from RUST_LOG=autocxx_engine=info cargo build when using with b.flag_if_supported("-std=c++17"). Notice -std=c++14 in log:


  cargo:warning=MESSAGE:Bindgen flags would be: "--use-specific-virtual-function-receiver" "--cpp-semantic-attributes" "--represent-cxx-operators" "--use-distinct-char16-t" "--blocklist-item" "std::unique_ptr" "--blocklist-item" "std::vector" "--blocklist-item" "std::shared_ptr" "--blocklist-item" "std::weak_ptr" "--blocklist-item" "std::string" "--blocklist-item" "rust::Str" "--blocklist-item" "rust::String" "--blocklist-item" "rust::Box" "--allowlist-type" "mbgl::ffi::Context::Context" "--allowlist-type" "autocxx_make_string_0x87976c9e4c91ff15" "--allowlist-function" "mbgl::ffi::Context::Context" "--allowlist-function" "autocxx_make_string_0x87976c9e4c91ff15" "--allowlist-var" "mbgl::ffi::Context::Context" "--allowlist-var" "autocxx_make_string_0x87976c9e4c91ff15" "--default-enum-style" "rust" "--enable-cxx-namespaces" "--no-layout-tests" "--no-derive-copy" "--no-derive-debug" "--no-derive-default" "--generate" "functions,types,vars,methods,constructors,destructors" "--generate-inline-functions" "--rust-target" "1.68" "--respect-cxx-access-specs" "--" "-x" "c++" "-std=c++14" "-DBINDGEN"
... 

To Reproduce
Add .flag_if_supported("-std=c++17") to build.rs. Alternatively use .flag("-std=c++17")

Expected behavior
"-std=c++17" is applied to compiler invocation instead of "-std=c++14"

@jwinarske
Copy link
Author

If I use

    let mut b = autocxx_build::Builder::new("src/lib.rs", &[...])
        .extra_clang_args(&["-std=c++17", "-Wc++17-extensions", "-Wunused-parameter"])
        .build()?;

    b.file("cxx/context.cpp")
        .compile("example");

it appends "-std=c++17" but "-std=c++14" is still present.

@jwinarske
Copy link
Author

works fine.

@jwinarske jwinarske reopened this Jun 25, 2023
@jwinarske
Copy link
Author

jwinarske commented Jun 25, 2023

Actually it doesn't work.

Setting c++ version using extra_clang_args ends up as the last command line option. Which won't affect any of the include files.

It looks like autocxx itself is dependent on 14, as changing engine/src/lib.rs from "-std=c++14" to "-std=c++17" in local folder (using relative path to point to autocxx and autocxx-build) ends up with a build error.

This is the error I get when I update both autocxx and autocxx-build to -std=c++17:

warning: In file included from /home/joel/work/app/maplibre-native/ffi/target/debug/build/maplibre_ffi-78ae8e6a3d2fea4a/out/autocxx-build-dir/cxx/gen0.cxx:2:
warning: /home/joel/work/app/maplibre-native/ffi/target/debug/build/maplibre_ffi-78ae8e6a3d2fea4a/out/autocxx-build-dir/include/autocxxgen_ffi.h:35:78: error: ‘::rust’ has not been declared
warning:    35 | inline std::unique_ptr<std::string> autocxx_make_string_0xef637c73314d0a8e(::rust::Str str) { return std::make_unique<std::string>(std::string(str)); }
warning:       |                                                                              ^~~~
warning: /home/joel/work/app/maplibre-native/ffi/target/debug/build/maplibre_ffi-78ae8e6a3d2fea4a/out/autocxx-build-dir/include/autocxxgen_ffi.h:35:93: error: expected ‘,’ or ‘;’ before ‘{’ token
warning:    35 | inline std::unique_ptr<std::string> autocxx_make_string_0xef637c73314d0a8e(::rust::Str str) { return std::make_unique<std::string>(std::string(str)); }
warning:       |                                                                                             ^
warning: /home/joel/work/app/maplibre-native/ffi/target/debug/build/maplibre_ffi-78ae8e6a3d2fea4a/out/autocxx-build-dir/cxx/gen0.cxx: In function ‘std::string* cxxbridge1$autocxx_make_string_0xef637c73314d0a8e(rust::cxxbridge1::Str)’:
warning: /home/joel/work/app/maplibre-native/ffi/target/debug/build/maplibre_ffi-78ae8e6a3d2fea4a/out/autocxx-build-dir/cxx/gen0.cxx:149:96: error: cannot convert ‘std::unique_ptr<std::__cxx11::basic_string<char> >’ to ‘std::unique_ptr<std::__cxx11::basic_string<char> > (*)(rust::cxxbridge1::Str)’ in initialization
warning:   149 |   ::std::unique_ptr<::std::string> (*autocxx_make_string_0xef637c73314d0a8e$)(::rust::Str) = ::autocxx_make_string_0xef637c73314d0a8e;
warning:       |                                                                                              ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
warning:       |                                                                                                |
warning:       |                                                                                                std::unique_ptr<std::__cxx11::basic_string<char> >

This is just a simple scenario of trying to use #include <optional> which is only available with >= -std=c++17

@adetaylor
Copy link
Collaborator

You're right to be using extra_clang_args.

Applying this diff to the code in demo seems to build OK for me:

diff --git a/demo/build.rs b/demo/build.rs
index 69c632c4..f91eae0d 100644
--- a/demo/build.rs
+++ b/demo/build.rs
@@ -8,8 +8,10 @@
 
 fn main() -> miette::Result<()> {
     let path = std::path::PathBuf::from("src");
-    let mut b = autocxx_build::Builder::new("src/main.rs", [&path]).build()?;
-    b.flag_if_supported("-std=c++14").compile("autocxx-demo");
+    let mut b = autocxx_build::Builder::new("src/main.rs", [&path])
+        .extra_clang_args(&["-std=c++17"])
+        .build()?;
+    b.flag_if_supported("-std=c++17").compile("autocxx-demo");
 
     println!("cargo:rerun-if-changed=src/main.rs");
     println!("cargo:rerun-if-changed=src/input.h");
diff --git a/demo/src/input.h b/demo/src/input.h
index b6cdd874..5cc93955 100644
--- a/demo/src/input.h
+++ b/demo/src/input.h
@@ -12,6 +12,7 @@
 #include <sstream>
 #include <stdint.h>
 #include <string>
+#include <optional>
 
 class Goat {
 public:

@adetaylor
Copy link
Collaborator

(At the very least the documentation needs to be improved here, so I'll keep this issue open. But I want to make sure you can resolve your problem first - can you try the above diff against the demo, and report back on whether it works for you?)

@jwinarske
Copy link
Author

@adetaylor Using your suggest pattern works! The only combo I didn't try :)

This gets me past the issue:

    let mut b = autocxx_build::Builder::new("src/lib.rs", &[
        &path27, &path28, &path29, &path30,
        &path1, &path2,                          &path5, &path6, &path7, &path8, &path9, &path10,
        &path11, &path12, &path13, &path14, &path15, &path16, &path17, &path18, &path19, &path20,
        &path21, &path22, &path23, &path24, &path25
        ])
        .extra_clang_args(&["-std=c++17"])
        .build()?;

    b.flag_if_supported("-std=c++17").file("cxx/context.cpp")
        .compile("autocxx-maplibre-example");

Yes I agree the docs need to improve around this

@adetaylor
Copy link
Collaborator

OK great, thank you.

@adetaylor
Copy link
Collaborator

There was already a test for C++17 compatibility, so I just improved the documentation here. Thanks again for the report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants