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

Canonicalize include path in Windows to not contain UNC prefix #33

Merged
merged 1 commit into from
Oct 15, 2020
Merged

Canonicalize include path in Windows to not contain UNC prefix #33

merged 1 commit into from
Oct 15, 2020

Conversation

Bromeon
Copy link
Contributor

@Bromeon Bromeon commented Oct 14, 2020

Thanks for the fast fix of #27!

I would be shocked if this builds under Windows even after this is fixed. I anticipate that a load of bindgen configuration is necessary which isn't currently possible with the limited interface exposed by autocxx. (There's the AUTOCXX_INC environment variable to set an include path, but currently no other way to configure bindgen at all. It's my intention eventually that there are richer facilities here.)

I have shocking news for you. After some tweaking, I ran the Demo example:

cargo run
   Compiling autocxx-demo v0.2.0 (...\autocxx\demo)
    Finished dev [unoptimized + debuginfo] target(s) in 7.79s
     Running `...\autocxx\target\debug\autocxx-demo.exe`
Hello, world! - C++ math should say 12=12

Process finished with exit code 0

But from the beginning. The compilation step failed, and unfortunately the cc output is not that great -- it outputs the return code of MSVC compiler cl, however not the error message. I had to figure out which command failed, and reproduce it in the command line (which is not exactly the same setup, because cc loads the environment, e.g. to find standard headers).

Anyway, I found out that the include directory created by std::fs::canonicalize() on Windows is a UNC path, with a prefix \\?\ that is not understood by many Microsoft tools, among others cl. There is an existing issue rust-lang/rust#42869 on the Rust repo about this.

So I need to strip that prefix, and came across the dunce crate which did exactly that. It is of course also possible to write some inline code to not impose an extra dependency.


So far, so good, however I wasn't able to use autocxx from outside.
When I try to reproduce the Demo setup in a separate project, I get:

Compiling autocxx-test v0.1.0 (...\autocxx-test)
error: NoAutoCxxInc
   --> src\main.rs:17:1
    |
17  | include_cxx!(Header("input.h"), Allow("DoMath"),);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ in this macro invocation
    | 
   ::: ...\autocxx\src\lib.rs:106:1
    |
106 | pub fn include_cxx(input: TokenStream) -> TokenStream {
    | ----------------------------------------------------- in this expansion of `include_cxx!`

error[E0433]: failed to resolve: use of undeclared crate or module `ffi`
  --> src\main.rs:22:9
   |
22 |         ffi::cxxbridge::DoMath(4)
   |         ^^^ use of undeclared crate or module `ffi`

I saw the NoAutoCxxInc enumerator and was wondering, if the AUTOCXX_INC env var might be missing. I set it as AUTOCXX_INC=src (the relative path to input.h from the project root) but then got another error:

error: non-foreign item macro in foreign item position: include
   --> src\main.rs:17:1
    |
17  | include_cxx!(Header("input.h"), Allow("DoMath"),);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ in this macro invocation
    | 
   ::: ...\autocxx\src\lib.rs:106:1
    |
106 | pub fn include_cxx(input: TokenStream) -> TokenStream {
    | ----------------------------------------------------- in this expansion of `include_cxx!`

error[E0433]: failed to resolve: use of undeclared crate or module `cxx`
   --> src\main.rs:17:1
    |
17  | include_cxx!(Header("input.h"), Allow("DoMath"),);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    | |
    | use of undeclared crate or module `cxx`
    | in this macro invocation
    | 
   ::: ...\autocxx\src\lib.rs:106:1
    |
106 | pub fn include_cxx(input: TokenStream) -> TokenStream {
    | ----------------------------------------------------- in this expansion of `include_cxx!`

Is there something obvious I'm doing wrong, or what setup would autocxx require? Is this the extra bindgen configuration you talked about?

@google-cla
Copy link

google-cla bot commented Oct 14, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@Bromeon
Copy link
Contributor Author

Bromeon commented Oct 14, 2020

@googlebot I signed it!

@adetaylor
Copy link
Collaborator

Thanks a lot for this! I'll take a look at the dunce canonicalization and check that it presents no problems on other platforms.

And I am indeed shocked that you've got as far as you have. I have personally not yet been brave enough to even try to use autocxx in a real build system (I'm just on the cusp though.)

Meanwhile though - I think your problem might be as simple as this. Your project needs to depend on both autocxx and cxx. Try that? That may be something that can be resolved in future, but give it a try.

@adetaylor adetaylor merged commit a454242 into google:main Oct 15, 2020
@Bromeon Bromeon deleted the bugfix/windows-include-paths branch October 15, 2020 06:31
@Bromeon
Copy link
Contributor Author

Bromeon commented Oct 15, 2020

Thank you! 🙂

I tried to use both cxx and autocxx, same error. When I explicitly specify AUTOCXX_INC=src (I thought I tried that before...), I could get one step further to a MSVC linker error:

cargo run
   Compiling autocxx-test v0.1.0 (...\autocxx-test)
error: linking with `link.exe` failed: exit code: 1120
  |
  ...
note: Non-UTF-8 output: autocxx_test.33sv6zz2crkw1k82.rcgu.o : error LNK2019: unresolved external symbol
  \"cxxbridge05$DoMath\" in function \"_ZN12autocxx_test3ffi9cxxbridge6DoMath17h0302c8bc1b7f960cE\".
...\\autocxx-test\\target\\debug\\deps\\autocxx_test.exe : fatal error LNK1120: 1 unresolved external symbol

I assume linking needs to be done on my side (but didn't see it in the Demo example)? How would I know the name and location of the library?

Also, there seems to be no .cpp file, just a header input.h with an inline function definition (not only declaration) of DoMath(). I assume this was just for simplicity, and not meant as "the way you should use it".

@adetaylor
Copy link
Collaborator

Great. To confirm, you're now trying to use this outside of Cargo, right? Yes, you will need:

Then you should indeed get as far as the Rust side of the bindings building correctly, and you have! That's good news.

But....

Unfortunately the idea of cxx (and therefore autocxx) is that it generates both Rust and C++ side code in order to enforce safety and allow safe exchange of rich datatypes between Rust and C++. Therefore, we also need to generate some C++ code too. See [https://github.com/adetaylor/cxx/blob/book/book/src/building.md](the forthcoming cxx book) for an overall diagram of how it works in cxx; the arrangement is similar in autocxx.

And... that's not possible yet for autocxx in a non-Cargo environment. I've filed #35 for this. The good news is that I happened to start on this yesterday. My day is crammed with meetings today, so I doubt I'll get this pushed within the next 24 hours, but it should be within the next 48.

In case you feel like you're on a treadmill of discovering unimplemented features: you're right :) But here is a list of other things I expect you to come across.

  • cxx generates only a single C++ file; autocxx sometimes generates two. You'll need to take one or two output C++ files and put them into your build system. This is awkward with some build systems which prefer very static lists of files to build (including the one I care about) so I am going to have to do something to make this file list predictable as part of Fill out command line generation tool #35.
  • I have made no effort with things like #pragma once in the generated header file. It's trivial to fix, but may need fixing for anything other than the simple Cargo-based builds. Expect some light doom.
  • As I mentioned before, I don't believe AUTOCXX_INC is sufficient configuration for bindgen. In a real project, you're bound to need to specify things like the C++ version (11, 17 etc.) or a bunch of other LLVM options. There's no ability to do that right now. See here for my general thoughts about how this should be implemented. I'll be interested in your requirements when you get this far.
  • There will eventually be a significant compatibility break with Alias all functions and symbols to a single namespace #37. Probably not soon.
  • The biggie: at the moment I don't believe any of this will work with realistically complicated structures, types or header files. I've done absolutely no stress testing against real projects yet. I'm getting close-ish on my side. By "stress testing" I mean answering all these questions:
    • Literal stress testing. Will anything turn out to be O(n^4) and die withe real headers? (I doubt there are problems here, all the hard work is done by bindgen already).
    • Can bindgen parse realistically complicated pre-existing header files without more configuration? (per the note above about AUTOCXX_INC being likely insufficient)
    • Can the autocxx code comprehend and convert the complex bindgen output from a real project, instead of the toy noddy examples generated from the test suite? I'm certain the answer will be no. There are ~40 permutations of C++ features tested in the test suite; I'm sure some realistic project headers will throw up a dozen permutations or features or quirks I haven't thought of.
    • Even once autocxx can convert all those bindgen definitions into valid input for cxx, is the result ergonomic to use? That is the biggest question of all... in particular the whole model kind of assumes it's OK for 99% of C++ structures to be opaque without field access, and therefore be owned by UniquePtr on the Rust side. It's not at all clear if that's a viable model yet with a realistic codebase. That's why autocxx is a bit... rough around the edges... at the moment, it's mostly a bit of an experiment to figure out the answer there!

After that, there will be some light at the end of the tunnel.

@Bromeon
Copy link
Contributor Author

Bromeon commented Oct 15, 2020

No, I still used it with Cargo, but a separate project that adds autocxx as Cargo dependency. I basically tried to extract the Demo example to a separate project.

Thanks for the write-up, and definitely don't feel pressured! autocxx looks already quite promising, and I'm currently not planning to import massive headers, but rather looking to write (from zero) some basic C++-Rust glue code that is simple but extensible.

@adetaylor
Copy link
Collaborator

Ah I see, in which case you should be able to use the stuff from build.rs in your own project. That should successfully build the C++ bits using the cc crate and include them in your project. Maybe you just missed the build.rs bits? Good luck!

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

Successfully merging this pull request may close these issues.

2 participants