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

Reduce compile time when build maturin from source distribution #1306

Closed
2 tasks done
messense opened this issue Nov 28, 2022 · 4 comments · Fixed by #1333
Closed
2 tasks done

Reduce compile time when build maturin from source distribution #1306

messense opened this issue Nov 28, 2022 · 4 comments · Fixed by #1333
Labels
enhancement New feature or request
Milestone

Comments

@messense
Copy link
Member

messense commented Nov 28, 2022

In such situation when maturin is built from source for reasons like no prebuilt wheel available, pep517 build only needs the maturin pep517 subcommand, we don't need to waste time building lots of dependencies for features like upload that won't be used.

We should be able to detect PEP 517 isolated build environment by Python interpreter path, it will contain build-env:

* https://github.com/pypa/build/blob/4475cf1bf2362cfba8dd8b10ac97b42d066db502/src/build/env.py#L99
* https://github.com/pypa/pip/blob/5f3f592c4581a059ff9de0fb8052ef5c6ef25fd4/src/pip/_internal/utils/temp_dir.py#L19

Slow to compile dependencies:

@messense messense added the enhancement New feature or request label Nov 28, 2022
@konstin
Copy link
Member

konstin commented Nov 28, 2022

With this, there could be a case where that wheel ends in the cache, and when the user runs pip install maturin they always get the minimal instead of the proper one.

What would work would be a separate maturin-core package as build backend like poetry-core, though i'm not sure if the benefits outweigh the complexity; platforms without prebuilt maturin are rare and there's the wheel cache so it only needs to be built once

@messense
Copy link
Member Author

With this, there could be a case where that wheel ends in the cache, and when the user runs pip install maturin they always get the minimal instead of the proper one.

Good point, but IMO in this case they are unlikely to run pip install maturin directly since development usually happens on platforms with prebuilt wheels.

What would work would be a separate maturin-core package as build backend like poetry-core, , though i'm not sure if the benefits outweigh the complexity

I also thought about this and came to the same conclusion.

I think adding a env var like MATURIN_SETUP_ARGS to setup.py might be a acceptable middle ground, now user can explicitly ask for a smaller maturin build, since they ask for it they will be warned about the pip cache issue and it's their responsibility to manage cache.

@messense
Copy link
Member Author

messense commented Dec 4, 2022

On second thought, what if we just make bootstrap from sdist use --no-default-features by default? We'll still provide environment variable to customize features of course.

Most people should use prebuilt binary from PyPI or distro packaging channel, so bootstrap from sdist is often limited to

  1. Distro packaging when they want to package the PEP 517 build backend. We provide environment variable to allow feature customization.
  2. Install packages that use maturin as build backend on platforms that have no prebuilt wheel support. Use --no-default-features will be fastest, IMO the pip cache issue is not a big issue because even with --no-default-features it will still provides basic build, develop and sdist commands which is usually good enough for locally development.

@messense messense pinned this issue Dec 4, 2022
@messense messense changed the title Reduce compile time when build from source in PEP 517 isolated build environment Reduce compile time when build from source distribution Dec 4, 2022
@messense messense changed the title Reduce compile time when build from source distribution Reduce compile time when build maturin from source distribution Dec 4, 2022
@konstin
Copy link
Member

konstin commented Dec 4, 2022

yeah that sounds reasonable

@bors bors bot closed this as completed in 1e09e2f Dec 5, 2022
bors bot added a commit that referenced this issue Dec 6, 2022
1335: Remove deprecated config options r=messense a=messense



1336: cargo deny multiple crate versions r=messense a=messense

To avoid pulling in more of them in the future, part of #1306 

Co-authored-by: messense <[email protected]>
@messense messense unpinned this issue Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants