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

Fix pex warning #322

Merged
merged 1 commit into from
Feb 27, 2019
Merged

Fix pex warning #322

merged 1 commit into from
Feb 27, 2019

Conversation

jeffwidman
Copy link
Contributor

@jeffwidman jeffwidman commented Feb 12, 2019

I was using pex on an application that lists google-auth as a dependency and got the following warning:

..../pex/environment.py:330 UserWarning: The `pkg_resources` package was loaded from a pex
vendored version when declaring namespace packages defined by google-auth 1.6.2. The 
google-auth 1.6.2 distribution should fix its `install_requires` to include `setuptools`

So adding setuptools as a listed dependency to fix this.

Also making the listing alphabetical.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Feb 12, 2019
@jeffwidman
Copy link
Contributor Author

jeffwidman commented Feb 12, 2019

Note:
I am a bit unclear on the actual root cause of why pex is complaining about this.

But it seems like a harmless change as setuptools is the de-facto way to do things in python land these days.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Feb 12, 2019
@theacodes
Copy link
Contributor

This is fine, but can we set a minimum version on setuptools? It's unlikely this will work with any setuptools.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 19, 2019
@jeffwidman
Copy link
Contributor Author

jeffwidman commented Feb 27, 2019

This is fine, but can we set a minimum version on setuptools? It's unlikely this will work with any setuptools.

So I thought about that originally myself, but decided against it.

I agree that it's unlikely to work with all versions. However, let's think about what's happening here:

  1. The code is already calling pkg_resources, so if an invalid version of setuptools was used, this would already be failing.
  2. Therefore, all this PR does is make an implicit dependency explicit to silence a warning.
  3. So the practical reality is that all the versions of setuptools that your users are fetching will work.
  4. And if not, you'd be already seeing other bug reports.

Given the above, it would be arguably more confusing to pin a specific minimum version, because that implies there's something different about whatever arbitrary version we pick.

That said, this is your library, so if you want me to pick a specific version, I'm happy to, I just am not sure it makes sense here unless you already have reports of known bugs with older versions of setuptools.

@theacodes
Copy link
Contributor

theacodes commented Feb 27, 2019 via email

@jeffwidman
Copy link
Contributor Author

Is there a minimum setuptools version that you want to require?

I'm not very familiar with those bugs (or the packaging of namespaced packages in general), so happy to defer to your expertise.

Also, I'm not clear what level of "bugginess" you're comfortable tolerating. For example, if it's a bug in a recent version, but an edge case that only manifests in certain rare environments, you may want to allow it so that users who aren't able to upgrade their environment but also aren't affected by the edge-case bug aren't excluded from using this library. Or you may not, I don't know...

@theacodes
Copy link
Contributor

theacodes commented Feb 27, 2019 via email

@jeffwidman
Copy link
Contributor Author

Gotcha.

Grep'ing https://setuptools.readthedocs.io/en/latest/history.html for "namespace" shows 40.3.0 solved a problem with pkg_resource-style namespaces (pypa/setuptools#1321).

That said, 40.3.0 was released Sept 16, 2018 which is fairly recent...

If that's too new, 38.2.2 from Nov 27, 2017 fixed another bug (pypa/setuptools#1214 solved by pypa/setuptools#1215).

Thoughts?

@theacodes
Copy link
Contributor

theacodes commented Feb 27, 2019 via email

I was using `pex` on an application that lists `google-auth` as a dependency and got the following warning:

    ..../pex/environment.py:330 UserWarning: The `pkg_resources` package was loaded from a pex vendored version when declaring namespace packages defined by google-auth 1.6.2. The google-auth 1.6.2 distribution should fix its `install_requires` to include `setuptools`

So adding `setuptools` as a listed dependency to fix this.

Version `40.3.0` was chosen because it fixed a bug in the handling of
`pkg_resource`-style namespaces
(pypa/setuptools#1321). For more details on
why this version was picked, see the discussion in
#322.

Also making the listing alphabetical.
@jeffwidman
Copy link
Contributor Author

Sounds good. Updated.

@theacodes theacodes merged commit 908da75 into googleapis:master Feb 27, 2019
@jeffwidman jeffwidman deleted the patch-1 branch February 27, 2019 18:18
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants