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

Vendor nspektr. Utilize it in Distribution._install_dependencies. #3170

Merged
merged 3 commits into from Mar 13, 2022

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Mar 13, 2022

Summary of changes

In python/importlib_metadata#368, I was exploring ways to make entry point dependency resolution available outside of Setuptools. In doing so, I explored creating the nspektr project. In creating that project and writing its tests, I discovered a flaw in how EntryPoint.extras resolved (python/importlib_metadata#369), a flaw that affects Setuptools installation logic.

While the flaw is fixed in a late release of importlib_metadata, there are likely older versions of importlib_metadata as well as older versions of importlib.metadata without the fix (at the time of this writing, the bug still exists in all Python versions). I've used nspektr to provide a workaround for the flaw (jaraco/nspektr@7245f07).

This PR replaces the inline logic in Setuptools with the more thoroughly tested logic of nspektr, adopting the workaround for the broken extras handling and simplifying Setuptools own logic.

Pull Request Checklist

@jaraco jaraco merged commit ba25a5f into main Mar 13, 2022
@jaraco jaraco deleted the feature/nspektr branch March 13, 2022 18:01
@hroncok
Copy link
Contributor

hroncok commented Mar 17, 2022

I am a little worried about yet another vendored dependency, this time even a project without a meaningful readme.

@jaraco
Copy link
Member Author

jaraco commented Mar 17, 2022

I respect that. I can give the project a readme.

Ultimately, I'd like to move all vendored dependencies to regular dependencies, though I'm not sure that's possible any time soon.

@hroncok
Copy link
Contributor

hroncok commented Mar 17, 2022

Sure, moving all vendored dependencies out is a meaningful goal. But it used to be just a handful, now we have plenty. Let's not keep adding new ones before we move all vendored dependencies to regular dependencies?

@jaraco
Copy link
Member Author

jaraco commented Mar 17, 2022

Let's not keep adding new ones before we move all vendored dependencies to regular dependencies?

I'd originally set out to add this functionality to importlib.metadata, but couldn't do that because importlib.metadata can't depend on packaging. Similarly, I couldn't add it to packaging because it can't depend on importlib.metadata. So another package was necessary if the functionality was going to have use outside of setuptools (which I'd intended for it to do since I discourage use of setuptools except as a backend). Furthermore, I was in no rush to replace the behavior as found in Setuptools with the behavior in nspektr until the creation of nspektr and its tests revealed a flaw in Setuptools' own implementation.

Then I was left with the choice to either copy the fix into setuptools and maintain two copies of the same functionality or vendor the package and rely on the same copy. I've got far too little time to be able to afford maintaining multiple forks of the same thing, so vendoring it was. I really feel like I have little choice.

I don't think it's been said, though, what's the harm in having an additional package vendored?

@abravalheri
Copy link
Contributor

Let's not keep adding new ones before we move all vendored dependencies to regular dependencies?

Hi @hroncok , I think I can appreciate the concerns here, however there are some extra dependencies planned to be vendored in the near future to support features like pyproject.toml metadata (e.g. tomli). These have been proposed a few months ago. I hope that is OK.

@hroncok
Copy link
Contributor

hroncok commented Mar 17, 2022

Well, I am no authority here. I've made my point, but I am not willing to die on this hill :)

@jaraco
Copy link
Member Author

jaraco commented Mar 17, 2022

Miro, sorry if I've upset you. I had hoped that Setuptools could add dependencies as needed without adding complications. It seems from your sentiment that adding dependencies creates concerns for you. In #2825, I laid out the reasons why Setuptools wishes not to vendor dependencies, but none of my reasons seem particularly pertinent to a user or downstream packager. I would genuinely like to hear you articulate what motivated you to express your opinion, as that could influence how we think about new dependencies going forward.

@hroncok
Copy link
Contributor

hroncok commented Mar 18, 2022

Nah, I'm not upset. Just a bit confused about what setuptools is turning into.

With more vendored dependencies, setuptools grows bigger and bigger every release. We had a regression test asserting the wheel is no bigger than 600 KiB and we had to recently relax that requirement. The overall license of this package gets more complicated. There are more LICENSE files that we probably should ship with the package. The overall license of Python gets more complex, as it includes setuptools wheel...

(Most of this has already happened prior to this PR, e.g. this one particular dependency does not increase the size much or does not add yet another different license, but I speak generally about new vendored deps.)

It seems to me like this project has found itself smashed between 2 different forces. On one hand, it is the default legacy way how to build packages when you install them with pip or with python setup.py ... invocation. On other hand, it shall be a modern PEP 517 backend with dependencies, toml support and whatnot. Maybe it shouldn't be both?

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.

None yet

3 participants