Skip to content

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

Merged
messense merged 3 commits intoPyO3:mainfrom
messense:tool-maturin
Aug 13, 2021
Merged

Use [tool.maturin] options from pyproject.toml in build command#605
messense merged 3 commits intoPyO3:mainfrom
messense:tool-maturin

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!)


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