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

Fix cargo test with extension-module feature. #2135

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aviramha
Copy link
Member

@aviramha aviramha commented Jan 31, 2022

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

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That closes a lot of issues 🎉 Thanks for this.

guide/src/faq.md Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

👍 thanks!

It would also be worth adding cargo test (and cargo run) n CI to one of the examples crates, so that we know this is working as expected!

@aviramha
Copy link
Member Author

aviramha commented Feb 1, 2022

Regarding current failure - in order to get Paramus I need interpreter config which currently is initialized only if derive configuration feature is enabled.
Not sure what's the best solution here..

@davidhewitt
Copy link
Member

add_extension_module_link_args should probably be #[cfg(feature = "resolve-config")] - and you can then pass &InterpreterConfig through to _add_extension_module_link_args.

@aviramha aviramha force-pushed the rustc-link-arg-bins branch 4 times, most recently from 8aef9a0 to 9d1ffb1 Compare February 1, 2022 09:33
@aviramha aviramha force-pushed the rustc-link-arg-bins branch from 9d1ffb1 to db53229 Compare February 1, 2022 09:54
@aviramha
Copy link
Member Author

aviramha commented Feb 1, 2022

Not sure about cargo run/test - they don't seem to have any. You suggest adding a sanity test there also and running it?

@davidhewitt
Copy link
Member

Yep exactly, can add something simple to prove it works 😊

@aviramha
Copy link
Member Author

aviramha commented Feb 1, 2022

I can't make an example fail on my setup for some reason (macOS, arm). Some help would be appreciated

@davidhewitt
Copy link
Member

See #2147

@aviramha
Copy link
Member Author

aviramha commented Feb 5, 2022

Thanks @davidhewitt. I can reproduce it using the example you made.
I'm now stuck at the compiler not letting it compile due to ignored flag -
invalid instructioncargo:rustc-link-arg-binsfrom build script ofmaturin-starter v0.1.0 (pyo3/examples/maturin-starter)The package maturin-starter v0.1.0 (maturin-starter) does not have a bin target.
I think we have to change the implementation on pyo3 build config to consider what it is compiling (test/binary). is that information available from a build script context tho?

@davidhewitt
Copy link
Member

Oh, yikes. I have to confess I don't know. Might be possible to extract it from the Cargo.toml?

@aviramha
Copy link
Member Author

aviramha commented Feb 5, 2022 via email

@adamreichold
Copy link
Member

But I don’t think we can extract what we’re compiling exactly?

Does checking the CARGO_BIN_NAME environment variable (whether it is empty or not) help with this?

@davidhewitt
Copy link
Member

I think we might be able to use CARGO_MANIFEST_DIR in conjunction with cargo metadata to figure out what the crate of the currently running build script contains (and maybe even which of those things depend on pyo3).

@007vasy
Copy link

007vasy commented Mar 8, 2022

when merge?

@aviramha
Copy link
Member Author

aviramha commented Mar 8, 2022

when merge?

The PR isn't ready yet because I didn't think it through enough. We need to find a way to set the flags only on specific builds, which isn't as easy as one would think. I'm currently swamped by work & life so if someone wants to keep this moving they're more than welcome.

@007vasy
Copy link

007vasy commented Mar 8, 2022

when merge?

The PR isn't ready yet because I didn't think it through enough. We need to find a way to set the flags only on specific builds, which isn't as easy as one would think. I'm currently swamped by work & life so if someone wants to keep this moving they're more than welcome.

no worries man, I don't have the expertise yet to help you out, I will try to figure out a workaround

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