Skip to content

curl: run tests in passthru #178869

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.

Sign up for GitHub

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

Merged
Artturin merged 4 commits into from
Jul 6, 2022
Merged

curl: run tests in passthru #178869

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
67d19f4
phpPackage.mkExtension: makeOverridable
Artturin Jun 24, 2022
6bce3fc
curl: use finalAttrs
Artturin Jun 24, 2022
47b58ff
curl: put tests in tests.withCheck
Artturin Jun 24, 2022
7141ab0
release: add tests.packageTestsForChannelBlockers.curl.withCheck as a…
Artturin Jun 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions pkgs/tools/networking/curl/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
, ocamlPackages
, phpExtensions
, python3
, tests
, fetchpatch
}:

# Note: this package is used for bootstrapping fetchurl, and thus
Expand Down Expand Up @@ -61,14 +63,14 @@ assert wolfsslSupport -> wolfssl != null;
assert zlibSupport -> zlib != null;
assert zstdSupport -> zstd != null;

stdenv.mkDerivation rec {
stdenv.mkDerivation (finalAttrs: {
pname = "curl";
version = "7.83.1";

src = fetchurl {
urls = [
"https://curl.haxx.se/download/${pname}-${version}.tar.bz2"
"https://github.com/curl/curl/releases/download/${lib.replaceStrings ["."] ["_"] pname}-${version}/${pname}-${version}.tar.bz2"
"https://curl.haxx.se/download/curl-${finalAttrs.version}.tar.bz2"
"https://github.com/curl/curl/releases/download/curl-${finalAttrs.version}/curl-${finalAttrs.version}.tar.bz2"
];
sha256 = "sha256-9Tmjb7RKgmDsXZd+Tg290u7intkPztqpvDyfeKETv/A=";
};
Expand Down Expand Up @@ -152,7 +154,10 @@ stdenv.mkDerivation rec {
CXX = "${stdenv.cc.targetPrefix}c++";
CXXCPP = "${stdenv.cc.targetPrefix}c++ -E";

doCheck = true;
# takes 14 minutes on a 24 core and because many other packages depend on curl
# they cannot be run concurrently and are a bottleneck
# tests are available in passthru.tests.withCheck
doCheck = false;
preCheck = ''
patchShebangs tests/
'' + lib.optionalString stdenv.isDarwin ''
Expand All @@ -177,16 +182,23 @@ stdenv.mkDerivation rec {
ln $out/lib/libcurl${stdenv.hostPlatform.extensions.sharedLibrary} $out/lib/libcurl-gnutls${stdenv.hostPlatform.extensions.sharedLibrary}.4.4.0
'';

passthru = {
passthru = let
useThisCurl = attr: attr.override { curl = finalAttrs.finalPackage; };
Comment thread
Artturin marked this conversation as resolved.
Outdated
in {
inherit opensslSupport openssl;
tests = {
inherit curlpp coeurl;
haskell-curl = haskellPackages.curl;
ocaml-curly = ocamlPackages.curly;
php-curl = phpExtensions.curl;
pycurl = python3.pkgs.pycurl;
withCheck = finalAttrs.finalPackage.overrideAttrs (_: { doCheck = true; });

@roberth roberth Jun 29, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This one should be a channel blocker if it fails. It should be added to release.nix for the nixpkgs and nixos jobs.

@Artturin Artturin Jun 29, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not sure i did it correctly

@Artturin Artturin Jun 29, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

error: attribute 'tests' missing

       at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/ofborg-evaluator-4/pkgs/top-level/release.nix:100:15:

           99|               jobs.stdenv.x86_64-linux
          100|               jobs.curl.tests.withCheck.x86_64-linux
             |               ^
          101|               jobs.cargo.x86_64-linux

are passthru attributes stripped?

@roberth roberth Jun 29, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah release.nix is kind of a mess (or maybe I'm unfamiliar) and it strips all packages down using hydraJobs. It's supposed to help that evaluator release memory.
If you add the passthru tests as "normal" packages somewhere in pkgs, you won't have to deal with the release.nix code as much.

Maybe do something like this in pkgs/test/default.nix?

corePackageTests = recurseIntoAttrs {
  curl = recurseIntoAttrs pkgs.curl.tests;
};

That should give you pkgs.tests.corePackageTests, which you can use in the release files.

fetchpatch = tests.fetchpatch.simple.override { fetchpatch = fetchpatch.override { fetchurl = useThisCurl fetchurl; }; };
curlpp = useThisCurl curlpp;
coeurl = useThisCurl coeurl;

@roberth roberth Jun 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

      fetchpatch = tests.fetchpatch.simple.override { fetchpatch = fetchpatch.override { fetchurl = useThisCurl fetchurl; }; };

where tests is pkgs.tests.

Haven't tried this, but would be pretty great to have something like this.
Also would be nice to have a direct fetchurl test. 🤷

@roberth roberth Jun 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't actually want to repeat .simple here, but I don't think we have an overridePackages to complement callPackages yet. That could use recurseForDerivations to do the right thing.

@risicle risicle Jun 29, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As rigorous as it is to ensure all these test-packages are built with this curl, I question slightly how relevant it is - most of these packages are only ever built with one curl, and may not actually build with esoteric/minimal curl builds. How is the tester to know what's supposed to build and what isn't?

It also slightly raises the question whether we're going to do this with all the reverse-dependency passthru.tests we've added. Strictly we should, but in reality it would end up with testers building all kinds of package variants that aren't usually expected.

haskell-curl = useThisCurl haskellPackages.curl;
ocaml-curly = useThisCurl ocamlPackages.curly;
pycurl = useThisCurl python3.pkgs.pycurl;
php-curl = useThisCurl phpExtensions.curl;
# error: attribute 'override' missing
# Additional checking with support http3 protocol.
inherit (nixosTests) nginx-http3;
# nginx-http3 = useThisCurl nixosTests.nginx-http3;
nginx-http3 = nixosTests.nginx-http3;

@roberth roberth Jun 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the future (#176557), nixosTests.nginx-http3.extend { defaults = { /* NixOS module */ }; }

(fyi)

Comment thread
Artturin marked this conversation as resolved.
Outdated
};
};

Expand All @@ -199,4 +211,4 @@ stdenv.mkDerivation rec {
# Fails to link against static brotli or gss
broken = stdenv.hostPlatform.isStatic && (brotliSupport || gssSupport);
};
}
})
6 changes: 3 additions & 3 deletions pkgs/top-level/php-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ lib.makeScope pkgs.newScope (self: with self; {
#
# Build inputs is used for extra deps that may be needed. And zendExtension
# will mark the extension as a zend extension or not.
mkExtension =
{ name
mkExtension = lib.makeOverridable
({ name
, configureFlags ? [ "--enable-${name}" ]
, internalDeps ? [ ]
, postPhpize ? ""
Expand Down Expand Up @@ -151,7 +151,7 @@ lib.makeScope pkgs.newScope (self: with self; {
description = "PHP upstream extension: ${name}";
inherit (php.meta) maintainers homepage license;
};
});
}));

php = phpPackage;

Expand Down