Vendoring updates (Oct 2022)

[pradyunsg]

Eventually closes #11500

• Solid nerd snipe on this one @pfmoore 😛
• @uranusjr Could you take a look at the certifi / requests patch changes? (asking since IIRC, you were the original author of the certifi.where patches we have)

Beyond that... I have no clue if this is actually going to work -- we'll let CI tell us that. I'm off to sleep now tho -- it's past 2:30am and I would like to get to my day job on time.

[pradyunsg]

Well, this is only mildly annoying. setuptools has changes the dependency graph for pkg_resources at some point. :)

https://github.com/pypa/setuptools/blob/d138ec08efc2dbaebb8752e215e324f38bd807a2/pkg_resources/_vendor/vendored.txt

[pradyunsg]

setuptools has changes the dependency graph for pkg_resources at some point. :)

Well, turns out, it's a bit more complicated than I'd hoped for -- https://github.com/pypa/setuptools/tree/2d4ebde71ce9daf914318f5ff29bcc39507b2cdf/pkg_resources/_vendor is a better source of truth than the requirements file in the parent directory.

[pradyunsg]

OK, 3am. That's all I'm investing into this, on this morning.

[pfmoore]

Well, this broke CI pretty badly. And it's a lot bigger than I'm comfortable with, so my first instinct is to reject it for 22.3, as we're too close to the release date for me to feel sure we've had the chance to iron out any issues.

More detailed notes:

1. The large number of extra dependencies for the setuptools change is more than I'm comfortable with. Are you just including dependencies of pkg_resources, or did you include dependencies of the "core" setuptools?
2. I'd prefer us to patch certifi to go back to the old importlib.resources API, as we know that's sufficient for our use case. When certifi converge on a better fix for the problem of the ever-changing importlib.resources API, we can pick that up and drop our patch.
3. Alternatively, given that we seem to be pulling in importlib_resources (for setuptools) why don't we just use that? Having said that, I'm arguing in (1) against keeping that extra vendoring, so unless I get overruled on (1), this won't be an option (no, I don't think we should vendor importlib_resources just for certifi).
4. The patch to remove brotli seems now to be removing some imports. That feels odd - why is this?

As a more general point, I'd much prefer to see this PR split into separate ones:

1. One PR with any upgrades that "just work" with no change to our patches and no new vendored libraries.
2. Each dependency that needs extra work in its own PR.

I would accept the first of these PRs, and we can then discuss the merits of the remaining ones individually.

Sorry, @pradyunsg, I know this is not how the vendoring tool works, so this makes extra work for you, but it matches our normal policy of keeping PRs small and focused...

[pfmoore]

The large number of extra dependencies for the setuptools change is more than I'm comfortable with. Are you just including dependencies of pkg_resources, or did you include dependencies of the "core" setuptools?

Oh, wow. I just looked at a recent pkg_resources. That's a 7-fold increase in size, and 86 files instead of 4. For what is, as far as I can tell, zero change in functionality for pip. Sorry, but I'd strongly prefer to pin to the old version, and make Python 3.12 a cutoff for no longer using setuptools at all.

And honestly, I'd be inclined to retire the pkg_resources backend altogether and vendor importlib_resources, so we're completely rid of setuptools. But the latter is more risky, because (AIUI) the backends are subtly different in some cases... And it's not related to this issue.

[pradyunsg]

From the code:

By default, pip uses importlib.metadata on Python 3.11+

So... it should be fine? I think the minor problem is that pkg_resources is imported to locate eggs, even when using importlib.metadata.

[pfmoore]

With #11503, nox -s vendoring -- --upgrade certifi urllib3 runs cleanly (upgrades everything except setuptools, certifi and urllib3).

[pradyunsg]

I'll split out setuptools and certifi from this PR, and file separate ones for them. I will note that I believe we ought to not skip certifi from the release though -- it's got updates to the certificates in addition to the (frustratingly necessary) code changes.

2. I'd prefer us to patch certifi to go back to the old importlib.resources API, as we know that's sufficient for our use case.
3. Alternatively, given that we seem to be pulling in importlib_resources (for setuptools) why don't we just use that?

And... I'd prefer to have less invasive patches instead, sticking to the wording in our policy around vendoring:

Vendored libraries MUST not be modified except as required to successfully vendor them.

Arguably, we're violating that for the certificate use case already -- my guess is that, downstream redistributors who would want to modify the certificates would much prefer the less invasive requests patch (in a file that's intended for patching) rather than the invasive certifi patch.

We ought to figure out a way to properly thread through the certificates in a follow up, that doesn't require patching our dependencies.

4. The patch to remove brotli seems now to be removing some imports. That feels odd - why is this?

It always did -- that hasn't changed.

brotli is a dependency that we can't vendor, due to binary dependencies, and we don't want to import it at runtime since it'd make that module uninstallable.

What's changed is that the lines around the code we were patching have changed enough that the patch was failing to apply. This PR updates that patch to apply cleanly on the newer urllib3.

Sorry, @pradyunsg, I know this is not how the vendoring tool works, so this makes extra work for you, but it matches our normal policy of keeping PRs small and focused...

FWIW, that sorry is unnecessary. I was considering doing the same already (I didn't coz it was 3am and I had to get to work at 9am).

---

All that will have to happen after my work day though -- Fridays are my open source days. :)

[pfmoore]

So... it should be fine?

I'm not sure what you mean by this. Fine to upgrade setuptools? Fine to not upgrade it?

IMO, it's clearly fine to not upgrade it, as we haven't had any problems using the out of date version until now. It's less obviously fine to upgrade it, because there's always a certain amount of risk, and even if it was fine, we would be adding a bunch of code for no benefit.

The original exclusing of setuptools was done by you in 1466e7c but I couldn't find a discussion of why this was done.

[pfmoore]

All that will have to happen after my work day though -- Fridays are my open source days. :)

No problem.

Would it help if I prepared a PR with everything but the brotli/certifi/setuptools changes?

[pradyunsg]

Would it help if I prepared a PR with everything but the brotli/certifi/setuptools changes?

Nah, it's fine -- it's easy enough to rebase this to remove those packages and cherry-pick them on a new branch off main. It's not much of a hassle, once I get in front of my personal laptop. :)

[pradyunsg]

I'm not sure what you mean by this. Fine to upgrade setuptools? Fine to not upgrade it?

Fine to not.

I'll try and elaborate on what I'm thinking later (it's related moving off of pkg_resources and how we discover eggs -- see find_eggs and the Environment class, in case you wanna look before then -- and #11501)

[pfmoore]

Nah, it's fine -- it's easy enough to rebase this to remove those packages and cherry-pick them on a new branch off main.

Too late, I did it anyway - #11504 😉 Feel free to ignore it if you want to do the lot yourself.

Anyway, I'll let you get back to work.