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

Use [tool.maturin] options from pyproject.toml in build command #605

Merged
merged 3 commits into from
Aug 13, 2021

Conversation

messense
Copy link
Member

@messense messense commented Aug 6, 2021

maturin build should respect these settings even when not building using PEP 517

available_options = [
"bindings",
"cargo-extra-args",
"compatibility",
"manylinux",
"rustc-extra-args",
"skip-auditwheel",
"strip",
]

The primary use case of this feature is that when a crate have an off-by-default optional feature say pyo3 to enable Python extension module, user can specify cargo-extra-args = "--features pyo3" in pyproject.toml to automatically "pass" them to maturin build/maturin publish

@messense messense changed the title Respect [tool.maturin] settings of pyproject.toml Use [tool.maturin] options from pyproject.toml in build command Aug 8, 2021
@messense messense force-pushed the tool-maturin branch 2 times, most recently from 9cec7b4 to 1031f77 Compare August 8, 2021 08:22
@messense messense marked this pull request as ready for review August 8, 2021 10:45
@davidhewitt
Copy link
Member

davidhewitt commented Aug 9, 2021

(going to try to review this later tonight) ... edit: gonna need to wait for another day, I'm exhausted 😴

Comment on lines 166 to 173
if cargo_extra_args.is_empty() {
// if not supplied on command line, try pyproject.toml
if let Some(args) = pyproject.and_then(|x| x.cargo_extra_args()) {
cargo_extra_args.push(args.to_string());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how to communicate to the user when some setting was loaded from tool.maturin.some-field and when the cli overwrote tool.maturin.some-field. Maybe it a generic "loading configuration from [tool.maturin]" and some message for which fields the cli overwrote the [tool.maturin] values. Otherwise it could be rather confusing if you e.g. git pull a project, run maturin build and then get an error that only occurs because [tool.maturin] overwrote some logic when the defaults would have worked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review. We certainly should inform user that we used configuration from [tool.maturin] section.

@messense
Copy link
Member Author

I think we can also drop the cli arguments passing on the Python PEP 517 support side now that maturin reads it on the Rust side, the toml Python dependency could be dropped then.

@konstin
Copy link
Member

konstin commented Aug 10, 2021

I think toml is still required for get_requires_for_build_wheel, but we can delete the [tool.maturin] part

@davidhewitt
Copy link
Member

(It sounds like you two have got this so I'm going to assume I'm not needed any more. Ping me if you do need me to review later!)

src/build_options.rs Outdated Show resolved Hide resolved

if !args_from_pyproject.is_empty() {
eprintln!(
"📡 Using build options {} from pyproject.toml",
Copy link
Member

Choose a reason for hiding this comment

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

💯 emoji choice

@anthrotype
Copy link

why can the --manifest-path option not be set from pyproject.toml, unlike the other maturin options? was it an oversight or a deliberate decision? Thanks for considering

haixuanTao added a commit to dora-rs/dora that referenced this pull request Aug 5, 2022
Since the merge of PyO3/maturin#605 ,
we can add features flag when maturin build the python project, removing
the need to always enabling `extension-module` flag when building.

The `extension-module` flag creates crash when building and testing outside
of maturin.

Previous fix required to disable default feature flag when building and testing which is not ideal.
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.

4 participants