feat(cli): ensure build backend is defined when running poetry build #10092
Conversation
Reviewer's Guide by SourceryThis pull request ensures that a build backend is defined in the pyproject.toml file when running Sequence diagram for poetry build command with build backend checksequenceDiagram
actor User
participant CLI
participant BuildCommand
participant PyProject
User->>CLI: poetry build
CLI->>BuildCommand: execute build
BuildCommand->>PyProject: check build-backend
alt No build backend defined
PyProject-->>BuildCommand: build-backend not found
BuildCommand-->>CLI: return error (1)
CLI-->>User: Show error message
else Build backend defined
PyProject-->>BuildCommand: build-backend found
BuildCommand->>BuildCommand: proceed with build
BuildCommand-->>CLI: return success (0)
CLI-->>User: Build completed
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
finswimmer
force-pushed
the
no-build-system-defined
branch
from
c1ce753 to
5634612
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please update the documentation to explain this new validation check and behavior change, since this changes how Poetry handles projects without a build-system defined.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
finswimmer
force-pushed
the
no-build-system-defined
branch
from
5634612 to
626abfc
Compare
finswimmer
changed the title
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please update the documentation to describe this new validation check for build backend configuration. This is a user-facing change that should be documented.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def build(self, options: BuildOptions) -> int: | ||
| if not self.poetry.is_package_mode: | ||
| self.io.write_error_line( | ||
| "Building a package is not possible in non-package mode." | ||
| ) | ||
| return 1 | ||
|
|
||
| if not self._has_build_backend_defined(): |
There was a problem hiding this comment.
suggestion: Consider making the error message more specific about whether the build-system section is missing or just the build-backend key
This would help users identify and fix configuration issues more quickly.
Suggested implementation:
def _get_build_system_status(self) -> tuple[bool, bool]:
pyproject_data = self.poetry.file.read()
has_build_system = "build-system" in pyproject_data
has_build_backend = "build-backend" in pyproject_data.get("build-system", {})
return has_build_system, has_build_backend has_build_system, has_build_backend = self._get_build_system_status()
if not has_build_system:
self.io.write_error_line(
"Missing [build-system] section in pyproject.toml. Please add this section with a build-backend."
)
return 1
if not has_build_backend:
self.io.write_error_line(
"Missing build-backend key in [build-system] section. Please define one in pyproject.toml."
)
src/poetry/console/commands/build.py
Outdated
| @@ -114,13 +114,22 @@ def _get_builder(self) -> Callable[..., None]: | |||
|
|
|||
| return self._build | |||
|
|
|||
| def _has_build_backend_defined(self) -> bool: | |||
| return "build-backend" in self.poetry.file.read().get("build-system", {}) | |||
There was a problem hiding this comment.
Would be good to move this to be something like self.poetry.pytpoject.build_system.is_fallback.
But that will need an additional property in https://github.com/python-poetry/poetry-core/blob/d97577e664d59173482a78f70e8f743cc5468851/src/poetry/core/pyproject/tables.py#L12
But for now something like this should work.
| return "build-backend" in self.poetry.file.read().get("build-system", {}) | |
| return "build-backend" in self.poetry.pyproject.data.get("build-system", {}) |
There was a problem hiding this comment.
Ah, I wasn't aware of the data attribute of pyproject 👍
I thought about a flag as well. But I don't wanted to change poetry-core before. And we would pollute again poetry-core with something we only need in poetry. Actually we only need it for poetry build.
| def build(self, options: BuildOptions) -> int: | ||
| if not self.poetry.is_package_mode: | ||
| self.io.write_error_line( | ||
| "Building a package is not possible in non-package mode." | ||
| ) | ||
| return 1 | ||
|
|
||
| if not self._has_build_backend_defined(): |
There was a problem hiding this comment.
Won't this also be a surprise to users as this breaks previous behavior in the same way setuptools are being used? Not sure how many people leave out the section intentionally.
That said, I'd also say that the current use of setuptools is in accordance with PEP518.
Tools should not require the existence of the [build-system] table. A pyproject.toml file may be used to store configuration details other than build-related data and thus lack a [build-system] table legitimately. If the file exists but is lacking the [build-system] table then the default values as specified above should be used. If the table is specified but is missing required fields then the tool should consider it an error.
There was a problem hiding this comment.
Won't this also be a surprise to users as this breaks previous behavior in the same way setuptools are being used?
It will surprise people for sure. But now they get an error message. Without it poetry build would build a package, but it won't contain the data people expect. So "fail fast" is a better surprise ;)
There was a problem hiding this comment.
The problem is, how do you determine if they intended to use setuptools by relying on PEP518 default?
It is not unreasonable to imagine that Poetry is used for dev but the package needs setuptools for build.
There was a problem hiding this comment.
But this is an edge enough case that it might be okay to enforce a build-system definition.
finswimmer
force-pushed
the
no-build-system-defined
branch
from
626abfc to
46f3513
Compare
it's not only "fine" - it's what PEP517 says tools should do.
only results in a broken package if the pyproject.toml is not understood by setuptools, presumably? In the new PEP621 world that is not going to be a problem. Perhaps a warning would make sense, but I suggest that erroring out where the specs say tools should not error out is overshooting. |
In the PEP 621 world, yes. But if the project doesn't (fully) use the As an alternative we could trigger a non-isolated build if no build-backend is given and (optional) show a warning that it would be good idea to add one to the |
|
Allowing the build to continue into isolated with a warning is safe enough, I reckon. Let's not add more logic unless we need it. That said I can already see the issues titled "why is poetry using setuptools" etc. |
Pull Request Check List
When we cannot find a
[build-system]section or abuild-backendwithin this section in apyproject.tomlwe assumesetuptoolsas the build backend This is fine according to PEP 517.While this approach is valid for dependencies it would be surprising on
poetry build, now that an isolated build happens if the build-system is notpoetry-core.Before #10059
poetry buildhad created the package even if no build backend was defined in thepyproject.toml. Now it would fallback tosetuptoolswhich would results in a broken package.This PR checks if a there is a
[build-system]section and has abuild-backenddefined in the pyproject.toml and would abort the build if it couldn't find it.Summary by Sourcery
Tests:
Summary by Sourcery
Tests:
pyproject.tomlfile.