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

add buildscript link flags only to parent crate #448

Merged
merged 2 commits into from
Oct 23, 2020

Conversation

dae
Copy link
Contributor

@dae dae commented Oct 16, 2020

@illicitonion Would you mind looking over this and seeing if it's sane? It's modifying code that was added in a previous pull request of yours: #346

I use a crate for creating Python extension modules called pyo3. It has code in it like this:

#[cfg_attr(windows, link(name = "pythonXY"))]
extern "C" {
    pub fn PyNode_Compile(arg1: *mut _node, arg2: *const c_char) -> *mut PyCodeObject;

Its build.rs script locates the relevant library name, and spits out a link flag that maps it to the alias:

-lpythonXY:python38

The pyo3 crate compiles successfully, but when I attempt to compile a crate that depends on pyo3 on Windows, the build fails, complaining that the above alias has been provided but there are no mentions of pythonXY in my crate - because the aliases are only in the pyo3 crate.

When I compile my crate using cargo instead of bazel, I can see that pyo3's build script link flags are not being passed in when compiling my crate - so the current behaviour of rules_rust does not seem to match what cargo is doing.

The change in this PR fixes the issue for me, and hasn't caused any problems here. But your PR specifically mentions transitive deps, so I wonder if I am missing something. Does this change cause a regression in the problem you were originally trying to fix?

@google-cla google-cla bot added the cla: yes label Oct 16, 2020
@damienmg
Copy link
Collaborator

Can we add an example that would fails without that change?

@dae
Copy link
Contributor Author

dae commented Oct 16, 2020

Have force-pushed a test without the original fix, and expect it to fail. Will follow up when test run is done.

Edit: the msvc change will require cargo raze is re-run, will follow up on that first.

@illicitonion
Copy link
Collaborator

Works for me - I added examples in #346 and if those are green, I'm happy :) Thanks!

On Windows, an -lalias:real link flag will cause an error when used
by a crate that does not link to the underlying library. This is
triggered when compiling a rust_library that has pyo3 as a dependency
on Windows.
@dae dae marked this pull request as ready for review October 16, 2020 21:11
@dae
Copy link
Contributor Author

dae commented Oct 16, 2020

Great, this should be good to go now.

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

Successfully merging this pull request may close these issues.

None yet

3 participants