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

Allow DSO export for C Rust bindings. #231

Merged
merged 2 commits into from
Jul 30, 2020

Conversation

adetaylor
Copy link
Collaborator

This option to the 'cxxbridge' command line tool allows
users to specify (for example)

__attribute__((visibility("default")))
or
__declspec(dllexport)

on C functions which may be exported from a shared object
as they may be required by Rust code in different binaries.

Fixes #219.

@adetaylor
Copy link
Collaborator Author

I don't consider this complete yet. I'm struggling to get the cxx test suite to run properly on my machine, then I want to figure out how to add a test for this.

@adetaylor adetaylor marked this pull request as draft July 22, 2020 00:27
@dtolnay
Copy link
Owner

dtolnay commented Jul 22, 2020

Sorry about the CI failure -- it looks like the compiler error tests in compiletest.rs were failing due to nightly having adjusted some of the diagnostics. I pushed a fix to master so hopefully the CI will pass after a rebase.

Locally, cargo test in the repo root is supposed to work. Let me know if it still causes trouble after rebasing. The compiler error tests will only run if using a nightly toolchain, but that's fine, it should skip them and still run the rest of the test suite and succeed on any other toolchain.

@adetaylor adetaylor force-pushed the set-visibility branch 2 times, most recently from a94f12b to 447c808 Compare July 22, 2020 05:03
@adetaylor
Copy link
Collaborator Author

Hi @dtolnay, a few questions about your preferences for the test code I've just added.

  1. Was there equivalent test code somewhere else I've missed?
  2. Do you consider these tests useful? (Obviously they could expand much more)
  3. How precisely do you feel they should analyze the C++ output? I am nervous about fragility even with these very vague tests, e.g. cxxbridge03 -> cxxbridge04. Do you feel I've struck a reasonable balance to make sure that something vaguely right is generated? We can obviously expand these tests easily in future to be more thorough.
  4. Can I or should I arrange for these to be run from the root test suite? (I'm not sufficiently familiar with crate organization to know if that's wise, or how to do it).

@adetaylor
Copy link
Collaborator Author

Incidentally, my thinking regarding adding new tests here is that it would be practically impossible to make compile tests which can also adequately test for different permutations of linking. That assumption may be wrong - let me know if you think that's the best direction to explore and I'll give it a try.

As to the CI errors, some versions of cargo are adding:

    = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>

It looks like some of the CI nightly builders do and some don't, so there is no permutation of code which will pass on all builders. Presumably this may fix itself within a week or so as they all roll to a more recent version of nightly.

adetaylor and others added 2 commits July 29, 2020 17:31
This option to the 'cxxbridge' command line tool allows
users to specify (for example)

  __attribute__((visibility("default")))
or
  __declspec(dllexport)

on C functions which may be exported from a shared object
as they may be required by Rust code in different binaries.
@dtolnay
Copy link
Owner

dtolnay commented Jul 30, 2020

It looks like some of the CI nightly builders do and some don't, so there is no permutation of code which will pass on all builders. Presumably this may fix itself within a week or so as they all roll to a more recent version of nightly.

Actually it looks like the reverse -- all the nightly builders used the most recently nightly: rustc 1.47.0-nightly (8ad7bc3f4 2020-07-21) (as I would expect; nightlies become available instantaneously around 01:00 UTC) but some of them used old versions of the PR, which is quite concerning and I have never seen happen before. Notice how https://github.com/dtolnay/cxx/runs/897461930?check_suite_focus=true#step:6:78 says it expected the compiler output to include that "to learn more" line, and the actual compiler output did not (matching the behavior of the most recent nightlies). But it shouldn't have expected that line because the build was triggered against 447c808 which was after your rebase over 6911d6d removing the line from the test suite. 😿

I'll push another rebase and keep alert for this happening again. According to https://www.githubstatus.com/ GitHub Actions had some degradation only a few hours later so it could have been some precursor of that...

Sorry about the confusing failures on this PR. :(

@dtolnay
Copy link
Owner

dtolnay commented Jul 30, 2020

To answer your questions on testing: I'll land this with the tests as currently implemented, but I filed #241 to try something more like what the Rust compiler repo does for its MIR tests.

@dtolnay dtolnay marked this pull request as ready for review July 30, 2020 00:37
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.

Add ability to set symbol visibility
2 participants