Skip to content

curl: patch certificate CN verification#167993

Merged
vcunat merged 1 commit intoNixOS:staging-nextfrom
PaulGrandperrin:fix-curl
Apr 10, 2022
Merged

curl: patch certificate CN verification#167993
vcunat merged 1 commit intoNixOS:staging-nextfrom
PaulGrandperrin:fix-curl

Conversation

@PaulGrandperrin
Copy link
Contributor

@PaulGrandperrin PaulGrandperrin commented Apr 9, 2022

Description of changes

From curl/curl@911714d
Fixes #167971

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:
  • 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.05 Release Notes (or backporting 21.11 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.

@PaulGrandperrin PaulGrandperrin mentioned this pull request Apr 9, 2022
Merged
13 tasks
@Mindavi
Copy link
Contributor

Mindavi commented Apr 9, 2022

LGTM, didn't test though. This fixes pycurl?

andrevmatos
andrevmatos reviewed Apr 9, 2022
View reviewed changes
gador
gador reviewed Apr 9, 2022
View reviewed changes
Copy link
Member

@gador gador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not possible. Curl needs to be build for fetchpatch. This will cause an infinite recusion.
There is a comment about that above the patch line.
This is correct as it is.

@gador
Copy link
Member

gador commented Apr 9, 2022

I can test later, but this LGTM.

@ofborg ofborg bot added the 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. label Apr 9, 2022
@ofborg ofborg bot requested a review from lovek323 April 9, 2022 14:20
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Apr 9, 2022
@PaulGrandperrin
Copy link
Contributor Author

PaulGrandperrin commented Apr 9, 2022

LGTM, didn't test though. This fixes pycurl?

nix build github:NixOS/nixpkgs/pull/167993/head#python3Packages.pycurl works fine

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Apr 9, 2022

Please target staging or staging-next if this is broken in the current cycle otherwise hydra will be busy for a while because of stdenv rebuild.

SuperSandro2000
SuperSandro2000 approved these changes Apr 9, 2022
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other than that LGTM. Give me a ping when you have retargeted and I'll merge it.

@PaulGrandperrin
Copy link
Contributor Author

PaulGrandperrin commented Apr 9, 2022

Please target staging or staging-next if this is broken in the current cycle otherwise hydra will be busy for a while because of stdenv rebuild.

I'm kinda new to NixOS release process, so I have no idea how to choose between staging and staging-next.
All I can say is that right now, I can't use nixos-unstable anymore because of this bug.

@PaulGrandperrin PaulGrandperrin changed the base branch from master to staging April 9, 2022 17:49
@PaulGrandperrin
Copy link
Contributor Author

PaulGrandperrin commented Apr 9, 2022

I read a little bit about staging and staging-next but I'm still not sure I fully grasped the consequences of choosing one over the other..
Anyway, for now, I changed this PR to target staging.

das-g
das-g approved these changes Apr 10, 2022
View reviewed changes
Copy link
Member

@das-g das-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I've tested nix build 'github:NixOS/nixpkgs/pull/167993/head#python3Packages.pycurl'.

Couldn't completely test

cd my/clone/of/nixpkgs
git switch nixos-unstable
git pull --ff-only
git cherry-pick 0fad2b34c4e1454ce83ec1486725771273c7c0dc
sudo nixos-rebuild --upgrade boot -I 'nixpkgs=.'

yet, as it would (as expected) re-build everything and the kitchen sink locally.

@gador
Copy link
Member

gador commented Apr 10, 2022 •
edited
Loading

I read a little bit about staging and staging-next but I'm still not sure I fully grasped the consequences of choosing one over the other.. Anyway, for now, I changed this PR to target staging.

Perfect. Targeting staging is the right branch for fixes which will cause mass-rebuilds (as seen by the labels...More than 5000 rebuilds for linux alone).

I tested the fix with nix build github:NixOS/nixpkgs/pull/167993/head#python3Packages.pycurl as well 👍

@SuperSandro2000 I think it can be merged

@vcunat vcunat changed the base branch from staging to staging-next April 10, 2022 14:58
@vcunat vcunat changed the title Patch curl certificate CN verification curl: patch certificate CN verification Apr 10, 2022
@vcunat vcunat merged commit 9da3fcf into NixOS:staging-next Apr 10, 2022
@vcunat
Copy link
Member

vcunat commented Apr 10, 2022

I tested that python3.pkgs.pycurl is fixed here. Patch is from upstream and minimal, so it seems safe enough.

@das-g
Copy link
Member

das-g commented Apr 10, 2022

Should the pycurl tests disabled in #166335 now be re-enabled?

vcunat added a commit that referenced this pull request Apr 10, 2022
@vcunat
Revert "python39Packages.pycurl: disable failing tests"
32f4270 
This partially reverts commit c270def from PR #166335.
The tests worked after curl fix (commit 0fad2b3 from PR #167993).
@vcunat
Copy link
Member

vcunat commented Apr 10, 2022

Yes, done.

Somehow we managed to order this the wrong way. The temporary disablement would still make sense on master, as it only rebuilds a package that fails anyway without the fix (+ transitive failures and rebuilds). So if someone wants to pursue that, but either way I'd hope that this gets to master fast as well (in 2-3 days).

@das-g
Copy link
Member

das-g commented Apr 10, 2022

Well, according to the PR tags, #166335 also causes > 1000 rebuilds, so it wouldn't be eligible to go directly to master, would it? Or would that be acceptable as a to-be-reverted workaround until we wait for the fix-proper here (with > 5000 rebuilds) to hit that branch?

@vcunat
Copy link
Member

vcunat commented Apr 10, 2022

The diff quite clearly affects only the pycurl package or anything that depends on it. And on Hydra that package fails. So if I didn't overlook anything, it should only rebuild those (transitive) failures.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. and removed 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Apr 10, 2022
@das-g das-g mentioned this pull request Apr 10, 2022
Merged
13 tasks
@das-g
Copy link
Member

das-g commented Apr 10, 2022

So if someone wants to pursue that

Here we go: #168154

vcunat added a commit that referenced this pull request Apr 10, 2022
@vcunat
Revert "python39Packages.pycurl: disable failing tests"
f44598b 
This partially reverts commit 7f8c513 from PR #168154,
and it's also the same as commit 32f4270.
The tests worked after curl fix (commit 0fad2b3 from PR #167993).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gador gador gador left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@Mindavi Mindavi Mindavi approved these changes

@lovek323 lovek323 Awaiting requested review from lovek323

+2 more reviewers

@andrevmatos andrevmatos andrevmatos left review comments

@das-g das-g das-g approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

python3Packages.pycurl building failed

7 participants

@PaulGrandperrin @Mindavi @gador @SuperSandro2000 @vcunat @das-g @andrevmatos