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

build: add circular dependency checker for build requirements #593

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

Conversation

jameshilliard
Copy link
Contributor

Implement a basic build requirement cycle detector per PEP-517:

  • Project build requirements will define a directed graph of requirements (project A needs B to build, B needs C and D, etc.) This graph MUST NOT contain cycles. If (due to lack of co-ordination between projects, for example) a cycle is present, front ends MAY refuse to build the project.

  • Front ends SHOULD check explicitly for requirement cycles, and terminate the build with an informative message if one is found.

See:
https://www.python.org/dev/peps/pep-0517/#build-requirements

src/build/__init__.py Outdated Show resolved Hide resolved
src/build/__init__.py Outdated Show resolved Hide resolved
src/build/__init__.py Outdated Show resolved Hide resolved
src/build/__init__.py Show resolved Hide resolved
src/build/__init__.py Outdated Show resolved Hide resolved
src/build/__init__.py Outdated Show resolved Hide resolved
src/build/_exceptions.py Outdated Show resolved Hide resolved
src/build/_util.py Outdated Show resolved Hide resolved
src/build/_util.py Show resolved Hide resolved
src/build/_util.py Outdated Show resolved Hide resolved
return metadata

# fallback to build_wheel hook
wheel = self.build('wheel', output_directory)
match = parse_wheel_filename(os.path.basename(wheel))
if not match:
raise ValueError('Invalid wheel')
self.project_name = match['distribution']
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very comfortable with mutating the builder in a bunch of different places. If we wanna revalidate the build deps I'd rather we reinitialised the builder by passing the dist name as an explicit parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not very comfortable with mutating the builder in a bunch of different places.

Yeah, this is def a bit messy, unfortunately we don't have a pep517 hook to get our own project name so I'm attempting to pull the name whenever possible.

If we wanna revalidate the build deps I'd rather we reinitialised the builder by passing the dist name as an explicit parameter.

Well re-validation can often be skipped, if we have a name in out pyproject.yaml project table we can grab it from there to do self cycle checks from the start. It's only when we can't determine our own project name before the build that we need to revalidate it AFAIU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we wanna revalidate the build deps I'd rather we reinitialised the builder by passing the dist name as an explicit parameter.

I'm thinking we should probably be validating the normalized project name in the various areas where it can potentially be retrieved are all the same and throw an exception if any are different. Right now there doesn't seem to be anything ensuring that a project name set via say the pyproject.toml project table matches the wheel, sdist and dist-info project names pulled via the regexes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented project name validation which will throw a ProjectNameValidationError exception if at any point a name update is inconsistent with the previous name update. This should ensure that builder project name state changes are done in a way that effectively locks in the normalized project name once one is available.

@jameshilliard jameshilliard force-pushed the circular-dep-detection branch 5 times, most recently from 9dc2175 to 7c77bf5 Compare March 26, 2023 03:26
src/build/__init__.py Outdated Show resolved Hide resolved
@jameshilliard jameshilliard force-pushed the circular-dep-detection branch 9 times, most recently from 3e659e0 to 55e242c Compare March 29, 2023 20:46
Implement a basic build requirement cycle detector per PEP-517:

- Project build requirements will define a directed graph of
requirements (project A needs B to build, B needs C and D, etc.)
This graph MUST NOT contain cycles. If (due to lack of co-ordination
between projects, for example) a cycle is present, front ends MAY
refuse to build the project.

- Front ends SHOULD check explicitly for requirement cycles, and
terminate the build with an informative message if one is found.

See:
https://www.python.org/dev/peps/pep-0517/#build-requirements
@gaborbernat
Copy link
Contributor

Seems it stalled, @jameshilliard let me know if you want to pick it up again and we can reopen.

@gaborbernat gaborbernat closed this Feb 1, 2024
@jameshilliard
Copy link
Contributor Author

Seems it stalled, @jameshilliard let me know if you want to pick it up again and we can reopen.

I think this was ready but just waiting on review, or were there unresolved issues?

@gaborbernat gaborbernat reopened this Feb 1, 2024
@gaborbernat
Copy link
Contributor

@jameshilliard can you rebase it against main; seem has merge conflicts now...

@jameshilliard
Copy link
Contributor Author

@jameshilliard can you rebase it against main

Sure, I'll try and take a look later today.

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.

3 participants