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

Improved ''setuptools.finalize_distribution_options'' hook. #2031

Conversation

KOLANICH
Copy link
Contributor

@KOLANICH KOLANICH commented Mar 18, 2020

Summary of changes

  • Improved entry points mechanism. Entry points now can contain JSON metadata appended to a ''name'' after ''@'' character. It is a backward-incompatible change, but the breakage will unlikely happen.
  • Now it allows the implementers to ast setuptools to provide them various information by adding a parameter with a certain name into the function signature. Old behavior is also supported. For the list of args names see ''DistributionoptionsFinalizationRemap'' in ''dist.py''.
  • Now it tolerates errors in hooks code, so a package with a broken hook doesn't prevent from updating itself.
  • Implemented ''setuptools.finalize_distribution_options'' hooks prioritization. A priority can be set as an integer or float metadata. Or, if you need to set other options via providing a ''dict'', it can be added into ''order'' key.
  • Now it allows a hook only be called if it is mentioned in ''setup'' args or in ''pyproject.toml''. It can be done by adding ''{"only": ["toml"]}'' metadata. This should reduce breakage due to malfunctions in ''setuptools'' or hooks code.

Closes #2030

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@KOLANICH KOLANICH force-pushed the finalize_distribution_options_improvements branch 12 times, most recently from 352334a to 7ea3f28 Compare March 18, 2020 18:08
…tadata appended to a ''name'' after ''@'' character. It is a backward-incompatible change, but the breakage will unlikely happen.
… stuff. When an entry point now fails to be loaded, it doesn't disrupt building process.
…setuptools.finalize_distribution_options hook
@KOLANICH KOLANICH force-pushed the finalize_distribution_options_improvements branch from 7ea3f28 to 033d886 Compare March 19, 2020 15:59
@KOLANICH KOLANICH force-pushed the finalize_distribution_options_improvements branch from 033d886 to 8d3e79a Compare March 19, 2020 16:46
@KOLANICH KOLANICH force-pushed the finalize_distribution_options_improvements branch from 8d3e79a to 59c68c2 Compare March 19, 2020 16:54
@jaraco
Copy link
Member

jaraco commented Mar 21, 2020

Thank you for putting this together. You've obviously put a lot of thought and work into addressing the issue.

My initial reaction is that this PR suggests a substantial change to the codebase to address what looks like a fairly simple concern. That is, if #2030 is the motivation for this issue, this PR is doing a lot more than just addressing that concern.

Moreover, the changelog entries seem to address multiple different issues. Was it your intention to include all of those changes in this PR?

@jaraco
Copy link
Member

jaraco commented Mar 21, 2020

Looking at the subsequent PRs, I see that they all are attempting to improve the finalize_distribution_options hook, but I worry that these attempts are adding quite a bit of behavior (complication) and maintenance burden to the code. At a high level, here are some concerns I would want to address before accepting changes like these:

  • The finalize_options code was previously only a few lines of code now no longer fits on a single screen.
  • Also, the entry point changes could have impact on other systems that implement entry points.
  • There's not much of a problem description or design. I'd like to see a bug report detailing a problem and a PR describing how this design solves that problem.
  • Ideally each change should take on a small scope. If one change is dependent on another, explain in the problem/design how one change is a prerequisite for another.
  • This code uses extensive use of if/else and try/except, adding a great deal of logical complexity and possible code flows. We should aim to limit the logical complexity of the project where possible.

The setuptools code is used across a very wide variety of systems and use-cases, so it's important for this project to be extremely deliberate about changes, including new features.

I don't want to discourage you - there may be some useful ideas here, but they're all entangled. Let's start by breaking the problem down into smaller chunks. I'm going to close this and the other PRs, but please do be encouraged to take another stab with a more modest change, and as we work together, perhaps we'll end up at a similar implementation.

@jaraco jaraco closed this Mar 21, 2020
@KOLANICH
Copy link
Contributor Author

KOLANICH commented Mar 21, 2020

Let's start by breaking the problem down into smaller chunks.

Haven't I already broken the problem into small chunks - the other PRs (#2037, #2032, #2033, #2034, #2035, #2036, #2038)? I mean some of them are small and straightforward. For example #2037 is useful even if all other PRs are rejected. Could you, please, review them all and find out which ones can and should be merged right now, and if not, name the issues preventing them from being merged and proposed ways to improve?

I'm going to close <...> the other PRs
There's not much of a problem description or design. I'd like to see a bug report detailing a problem and a PR describing how this design solves that problem.

I guess the best place to discuss the features are in the PRs, not in the issues. Because in the PRs there is code, so we can discuss specific pieces.

these attempts are adding quite a bit of behavior (complication) and maintenance burden to the code.

Yes, every feature is complication and maintaince burden.

Also, the entry point changes could have impact on other systems that implement entry points.

I know - it impacts my projects, using the same metadata mechanism (but it is very easy to support both old and new versions, just to check for presence of metadata field, and if there is no, parse the name ourselves). In fact I don't expect very big breakage. I mean I don't expect any non-alphanum names of entry points except the hacks for providing metadata, and if they use them, they probably should update to the new system.

So, despite these 2 drawbacks, the alternative to them is much more worse - to make all the developers of the projects using the hook to have a non-standard, probably own ones, impls, that can conflict each other. In fact I can move most of the code into a separate lib, that can be just used by setuptools plugins authors, but it has a drawback that it would be not a standard way to provide metadata, so it won't solve the problem of conflicts. So IMHO this really has to be addressed in setuptools itself.

As I see, there is already a mechanism (ordering) depending on metadata, though metadata is not yet implemented, so the mechanism doesn't work, so it seems that it is already decided that entry points should have metadata.

Also I feel like the addressing relative to setup.py is strongly needed. I mean I have made some tests and surprisingly found out that setuptools doesn't find setup.cfg, if called from another cwd, but IMHO it really should (though I have not yet implemented it).

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Mar 21, 2020

@jaraco , I have added some discussion on their purpose and design (except the one already told in #2031 (comment)) to the PRs.

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.

Cannot bootstrap.py
2 participants