python310Packages.urllib3: remove pyopenssl as it is no longer recommended

[dbarrosop]

Description of changes

Fixes #178954

Commit 2922699 introduced a bunch of optional dependencies as mandatory. This had the side effect of breaking urllib3 on darwin-aarch64 as pyopenssl isn't supported on that platform (#175875)

This PR introduces a couple of arguments to allow toggling optional dependencies on and off defaulting them to true with the exception of darwin-aarch64 which disables by default the broken optional dependencies.

Note: I am unsure if this is the right branch, I just tried to replicate what was done on the PR that introduced the change I linked above.

Note2: I think ideally we'd have as root packages:

• python310Packages.urllib3 (no optional deps, default installation, same as pip)
• python310Packages.urllib3-full (all optional deps enabled)

but didn't want to introduce "big" changes, just fix urllib3 on darwin-aarch64

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[azuwis]

Pyopenssl is marked broken on aarch64-darwin (#175875),
but it's no longer recommended for urllib3 (https://github.com/urllib3/urllib3/blob/main/src/urllib3/contrib/pyopenssl.py).
Removing pyopenssl from dependencies on aarch64-darwin will save near 9000 packages.
For reference, Debian make pyopenssl suggest for urllib3 (https://packages.debian.org/sid/python3-urllib3),
which means pyopenssl will NOT be installed when installing urllib3.

[SuperSandro2000]

Wrong target branch. This should target master or staging.

[SuperSandro2000]

Removing pyopenssl from dependencies on aarch64-darwin will save near 9000 packages.

We shouldn't make such dependency packages platform depended if upstream has no explicit support for it. It will only cause extra maintenance for all other packages.

[azuwis]

Removing pyopenssl from dependencies on aarch64-darwin will save near 9000 packages.

We shouldn't make such dependency packages platform depended if upstream has no explicit support for it. It will only cause extra maintenance for all other packages.

How about removing pyopenssl from urllib3 for all platforms?

From https://github.com/urllib3/urllib3/blob/main/src/urllib3/contrib/pyopenssl.py:

Module for using pyOpenSSL as a TLS backend. This module was relevant before
the standard library ssl module supported SNI, but now that we've dropped
support for Python 2.7 all relevant Python versions support SNI so
this module is no longer recommended.

The pyopenssl urllib3 backend was mainly written for python 2.7, python3-urllib3 does NOT need pyopenssl.

[SuperSandro2000]

How about removing pyopenssl from urllib3 for all platforms?

Yes, we should do that but it will cause a mass rebuild and I can't assess how big the breakage will be.

[dbarrosop]

Thanks for the review, I updated my PR to:

1. target staging
2. simply remove pyopenssl
3. changed the commit message/PR title

After reading the comments I wasn't sure if we still wanted to make the optional dependencies opt-in, happy to make the changes if you also want to do that.

[zmre]

(Just a note to say a ton of things have broken on my mac that I expect this to fix, so watching anxiously for this to make it through.)

[SuperSandro2000]

(Just a note to say a ton of things have broken on my mac that I expect this to fix, so watching anxiously for this to make it through.)

This will just fix packages that require urllib3 which are not that many. pyopenssl has still many other usages and this PR will probably not fix most of your issues.

[azuwis]

I think we should also remove cryptography and idna, as https://urllib3.readthedocs.io/en/stable/reference/contrib/pyopenssl.html say they are dependancies of pyopenssl.

I'm not sure about certifi though.

[dbarrosop]

I think I addressed all of your comments. Decided to comment the secure dependencies entirely as they don't seem relevant anymore. As mentioned by @azuwis, cryptography and idna are dependencies of pyopenssl and looking at certifi it seems they are only used in tests for pyopenssl [1]

[jraygauthier]

LGTM code-wise.

[ylh]

for those of you at home on aarch64-darwin eagerly awaiting this getting into unstable, the following overlay should do the trick for now:

_: { python3, lib, ... }: {
  python3Packages = (python3.override {
    packageOverrides = _: { urllib3, ... }: {
      urllib3 = urllib3.overrideAttrs ({ passthru, ... }: {
        passthru = passthru // {
          optional-dependencies =
            lib.filterAttrs (n: _v: n != "secure")
            passthru.optional-dependencies;
        };
        propagatedBuildInputs = passthru.optional-dependencies.brotli;
      });
    };
  }).pkgs;
}

This will just fix packages that require urllib3 which are not that many. pyopenssl has still many other usages and this PR will probably not fix most of your issues.

well, there's yt-dlp for starters

then, a cursory review of transitive build dependencies on urllib3 via sphinx on my machine alone:

• mpd
• qemu
• ghc
  • by extension, literally every single haskell package

i offer as friendly advice that dismissive handwaving in the comments of a PR that addresses breakage you introduced is a monumentally bad look, especially for a member of @NixOS

[sternenseemann]

Workaround for GHC has been merged #179181

[SuperSandro2000]

breakage you introduced

This just reflects upstreams support status and the issue that is open for 2.5 years. This already used many maintainer hours in some downstream packages where tests mysteriously broke because of this. Still I just used yet another hour for a proprietary OS that I don't even use.

---

Since I can't push to your branch I need to open a new PR. Also reverting #179181 in its entirety there. So moving this to #179505

Can't do that yet because master wasn't merged into staging, yet.

Also would love to know if that even builds on darwin but currently cmake boot is broken. Lets see later if it actually works.

[SuperSandro2000]

The rebuild amount is only so low because of #178700 which means we couldn't have merged this into master.

[dbarrosop]

Thanks for merging and for the thankless job that it is being an OSS maintainer, specially a project as massive and complex as nixpkgs.

Quick question, do you know when this will hit unstable approximately? I know it has to go through a few steps but I am not familiar with typical timeframes.

[SuperSandro2000]

Thanks for merging and for the thankless job that it is being an OSS maintainer, specially a project as massive and complex as nixpkgs.

❤️

Quick question, do you know when this will hit unstable approximately? I know it has to go through a few steps but I am not familiar with typical timeframes.

Since staging-next just merged (#178690) and I don't think there is a new PR yet, we could be lucky and based on the duration of the last staging-next run it could already be in 7 to 10 days but if problems happen it could also be 14 days.

[zmre]

+1 on the thank you for the otherwise thankless job here. I was one of the ones anxious to see this hit and I know working on other people's platforms is a pain. Thanks for getting it done.

[Atemu]

This PR is broken:

error: attribute 'secure' missing

       at /Users/Atemu/Projects/nixpkgs/pkgs/development/python-modules/urllib3/default.nix:29:8:

           28|   propagatedBuildInputs = passthru.optional-dependencies.brotli
           29|     ++ passthru.optional-dependencies.secure;
             |        ^
           30|
(use '--show-trace' to show detailed location information)

That obviously needs to be removed aswell.

@SuperSandro2000 is it intended that the socks are not part of propagatedBuildInputs anymore here?

[Atemu]

Ah, sorry. Fixed in f8d1cad

[SuperSandro2000]

Ah, sorry. Fixed in f8d1cad

that PR took all the mass rebuilds and it is reason why this still must go to staging.

[Atemu]

Yes of course. Though I think I would've preferred if an aarch64-darwin-only optionals flag made it to master first and staging then had a change to remove it for all platforms. All dependent programs (which are a ton) will now remain broken on aarch64-darwin for at least a few weeks which isn't great.

[SuperSandro2000]

Though I think I would've preferred if an aarch64-darwin-only optionals flag made it to master first and staging then had a change to remove it for all platforms.

Then we would at least already have one merge conflict someone would need to fix again.

for at least a few weeks which isn't great.

pyopenssl is marked broken for aarch64-darwin since 11. May which is almost 6 weeks ago. Likely it will be fixed in a few days with #179844 already. I think we have that time now.

[Atemu]

I meant to implement the darwin handling into master and then make a PR against staging where the optional darwin handling is replaced with the unconditional one/removal.

Now it's too late to do this without a merge conflict of course; I just would prefer to do it this way next time to reduce breakage time.

Workaround to master first, rebuild-heavy change that does it properly to staging.

pyopenssl is marked broken for aarch64-darwin since 11. May which is almost 6 weeks ago.

Only in staging. It's only been broken on master (where everyone would notice) since the last staging-next.

[SuperSandro2000]

The PR that marked pyopenssl as broken went straight to master #172397 .

If the recent staging-next merge broke this due to dependency changes then this reflects aarch64-darwin in the support tier (https://github.com/NixOS/rfcs/blob/master/rfcs/0046-platform-support-tiers.md#tier-7-1) where it is the lowest possible.

With support tier 7 I don't think that extra work is justified in any way but arguable aarch64-darwin should really be tier 4 or 3 rather than 7. I still wouldn't really like to go that extra round for such a low tier support platform.

I don't think it would be fair or justified to push it into support tier 2 and if my wishes would be heard we would also move x86_64-darwin to support tier 3 mostly because fixing i686 is miles easier than darwin.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-know-which-package-in-my-hm-config-depends-on-another/20163/9