Fix cargo test with extension-module feature.

[davidhewitt]

The excellent news is that rust-lang/cargo#8441 will resolve a lot of our existing issues with extension-module. The bad news is that it's not yet merged, so please wait with merging this for now!

I'm not sure if this solves #904 (proc-macro crate) - we'll have to test and perhaps refine. I was able to confirm locally that cargo test and cargo run are fixed with this feature.

Fixes #1084
Fixes #771
Fixes #341
Fixes #340

[programmerjake]

Will this prevent testing pyo3 code on rust stable due to -Zextra-link-arg being unstable?

[davidhewitt]

I don't think so, my experience just now was that cargo ignores the directive printed by the build script if it doesn't recognize it.

[gilescope]

@programmerjake its nice to have worries like that. So nice that pyo3 is available on stable. So much great work done this year!

[davidhewitt]

As the cargo PR just got merged, I've rebased this and once I've confirmed it works on Rust nightly I'll update docs and propose to merge this.

[Spirans]

Hello!
I really appreciate this project and worrying a little about this PR, because my team has exactly the same issue.
@davidhewitt so is everything fine with night build? Do you need some help with something about that PR?

[davidhewitt]

Hi @Spirans thanks for the check-in.

I rebased this PR again just now just to confirm what I last found. Basically we need some of the build-script variables from rust-lang/cargo#8441 (comment) for this PR: specifically at the moment this uses rustc-link-arg-bins, which fixes cargo run, but we also need rustc-link-arg-tests to fix cargo test.

According to rust-lang/cargo#8441 (comment) the PR which implemented rustc-link-arg-bins added "the plumbing" to support rustc-link-arg-tests, but didn't implement it.

I was planning to write an upstream cargo PR to implement rustc-link-arg-tests (I guess it should be quite easy as it should just be copying the rustc-link-arg-bins code and modifying a little). However I'm quite busy so didn't get around to writing that PR yet.

Do you need some help with something about that PR?

If you've got the time and want to push this PR through faster, would you be willing to write the cargo PR?

[Spirans]

@davidhewitt thank you!
I'll take a look at that fix and will probably implement rustc-link-arg-tests soon.

[davidhewitt]

FYI @Spirans I just had a chance this morning to write the cargo PR: rust-lang/cargo#9416

[svenstaro]

Any chance to get this cleaned up and merged?

[davidhewitt]

I will complete the cargo PR first (hopefully soon) and then once the support is there in Rust nightly I will finish this off.

[davidhewitt]

Some updates to this:

• The cargo PR I opened got closed due to inactivity 🤦 . I'll pick it up again soon and push it through, now that 0.14.2 fixes are out of the way.
• The learning from Tracking issue for rustc-cdylib-link-arg transitive warning rust-lang/cargo#9562 is that what this PR adds to PyO3's build scripts is not actually correct. Instead, these link args must be added to the final crate. Most likely, pyo3_build_config::add_extension_module_link_args() can have these added.

[aviramha]

The linked PR seems to be merged? Can we get this done? This will be great!

[davidhewitt]

Next step here is for someone to reboot my closed PR rust-lang/cargo#9416, taking on the feedback I received and see it across the finish line.

I've been meaning to do it for absolutely ages, and will do it eventually, but if anyone wants to volunteer to take that on then that'll definitely help this come sooner!

[aviramha]

Next step here is for someone to reboot my closed PR rust-lang/cargo#9416, taking on the feedback I received and see it across the finish line.

I've been meaning to do it for absolutely ages, and will do it eventually, but if anyone wants to volunteer to take that on then that'll definitely help this come sooner!

I'll pick it. Great way to contribute for the first time to Rust repositories !

[aviramha]

@davidhewitt see rust-lang/cargo#10274

[davidhewitt]

Brilliant, thank you!

[aviramha]

@davidhewitt Please check my last comment in rust-lang/cargo#10274 . I want to make sure I explained the use case correctly ;)

[aviramha]

It's merged now! I assume we need to wait for it to get to stable?

[davidhewitt]

Fantastic, thanks so much for getting this moving!

I'd be ok with finding a way for this feature to use nightly for now (it's easy enough to ask users who want this to use cargo +nightly test). We just need to wait for rust nightly to update the pinned cargo (which happens every few weeks I think).

Note that due to the way the extra-link-arg options now work, the approach in this PR isn't quite correct. Instead, the build script of the final crate needs to be the one which has these linker arguments.

I actually wrote pyo3_build_config::add_extension_module_link_args with exactly this in mind - so I suggest the next steps after this is available in nightly would be to add to that function to emit the additional link args added by this PR. @aviramha would you be interested in taking that on? I can explain in more detail as you need.

[aviramha]

Sure I can take it.

[aviramha]

Closing this draft, see new PR #2135