From 226fcae24a4e2e0f68e62ece2ec1621d260ad3cd Mon Sep 17 00:00:00 2001 From: Matthew Kenigsberg Date: Mon, 20 Nov 2023 11:04:45 -0700 Subject: [PATCH 1/2] feat(lock): better error messages on failure Instead of just saying that a group failed to resolve, say which package failed to resolve in which input. Also factored out some shared data in test cases. --- include/flox/resolver/environment.hh | 9 +- src/resolver/environment.cc | 131 ++++++++++------ tests/environment.cc | 222 +++++++++++++++------------ tests/test.hh | 4 + 4 files changed, 219 insertions(+), 147 deletions(-) diff --git a/include/flox/resolver/environment.hh b/include/flox/resolver/environment.hh index 5887e795..830645d5 100644 --- a/include/flox/resolver/environment.hh +++ b/include/flox/resolver/environment.hh @@ -52,13 +52,16 @@ namespace flox::resolver { /* -------------------------------------------------------------------------- */ -FLOX_DEFINE_EXCEPTION( ResolutionFailure, +FLOX_DEFINE_EXCEPTION( ResolutionFailureException, EC_RESOLUTION_FAILURE, "resolution failure" ); /* -------------------------------------------------------------------------- */ +using ResolutionFailure = std::vector>; +using ResolutionResult = std::variant; + /** * @brief A collection of data associated with an environment and its state. * @@ -172,7 +175,7 @@ private: * @return `std::nullopt` if resolution fails, otherwise a set of * resolved packages. */ - [[nodiscard]] std::optional + [[nodiscard]] ResolutionResult tryResolveGroup( const InstallDescriptors & group, const System & system ); /** @@ -181,7 +184,7 @@ private: * @return `std::nullopt` if resolution fails, otherwise a set of * resolved packages for. */ - [[nodiscard]] std::optional + [[nodiscard]] std::variant tryResolveGroupIn( const InstallDescriptors & group, const pkgdb::PkgDbInput & input, const System & system ); diff --git a/src/resolver/environment.cc b/src/resolver/environment.cc index 3f3f0901..89b876c8 100644 --- a/src/resolver/environment.cc +++ b/src/resolver/environment.cc @@ -410,6 +410,8 @@ Environment::getGroupInput( const InstallDescriptors & group, { if ( descriptor.group == oldDescriptor.group ) { + // TODO: check that input is still present in a + // registry somewhere? return maybeLockedPackage->input; } @@ -431,16 +433,18 @@ Environment::getGroupInput( const InstallDescriptors & group, } } } + // TODO: check that input is still present in a registry somewhere? return wrongGroupInput; } /* -------------------------------------------------------------------------- */ -std::optional +ResolutionResult Environment::tryResolveGroup( const InstallDescriptors & group, const System & system ) { + ResolutionFailure failure; if ( auto oldLockfile = this->getOldLockfile(); oldLockfile.has_value() ) { auto lockedInput @@ -451,22 +455,54 @@ Environment::tryResolveGroup( const InstallDescriptors & group, nix::ref store = this->getStore(); pkgdb::PkgDbInput input( store, registryInput ); auto maybeResolved = this->tryResolveGroupIn( group, input, system ); - if ( maybeResolved.has_value() ) { return maybeResolved; } + if ( const SystemPackages * resolved + = std::get_if( &maybeResolved ) ) + { + return *resolved; + } + else if ( const InstallID * iid + = std::get_if( &maybeResolved ) ) + { + failure.push_back( std::pair { + *iid, + input.getDbReadOnly()->lockedRef.string } ); + } + else + { + throw ResolutionFailureException( + "we thought this was an unreachable error" ); + } } } // TODO: Use `getCombinedRegistryRaw()' for ( const auto & [_, input] : *this->getPkgDbRegistry() ) { auto maybeResolved = this->tryResolveGroupIn( group, *input, system ); - if ( maybeResolved.has_value() ) { return maybeResolved; } + if ( const SystemPackages * resolved + = std::get_if( &maybeResolved ) ) + { + return *resolved; + } + else if ( const InstallID * iid + = std::get_if( &maybeResolved ) ) + { + failure.push_back( std::pair { + *iid, + input->getDbReadOnly()->lockedRef.string } ); + } + else + { + throw ResolutionFailureException( + "we thought this was an unreachable error" ); + } } - return std::nullopt; + return failure; } /* -------------------------------------------------------------------------- */ -std::optional +std::variant Environment::tryResolveGroupIn( const InstallDescriptors & group, const pkgdb::PkgDbInput & input, const System & system ) @@ -493,7 +529,7 @@ Environment::tryResolveGroupIn( const InstallDescriptors & group, { pkgRows.emplace( iid, maybeRow ); } - else { return std::nullopt; } + else { return iid; } } /* Convert to `LockedPackageRaw's */ @@ -530,50 +566,53 @@ Environment::lockSystem( const System & system ) auto groups = this->getUnlockedGroups( system ); /* Try resolving unresolved groups. */ + std::vector failures; + std::stringstream msg; + msg << "failed to resolve some package(s):" << std::endl; for ( auto group = groups.begin(); group != groups.end(); ) { - std::optional maybeResolved - = this->tryResolveGroup( *group, system ); - if ( maybeResolved.has_value() ) - { - pkgs.merge( *maybeResolved ); - group = groups.erase( group ); - } - else { ++group; } - } - - if ( ! groups.empty() ) - { - std::stringstream msg; - msg << "failed to resolve some packages(s):"; - for ( const auto & group : groups ) - { - // TODO tryResolveGroupIn should report which packages failed - // to resolve. - if ( auto descriptor = group.begin(); - descriptor != group.end() - && descriptor->second.group.has_value() ) - { - - msg << std::endl - << " some package in group " << *descriptor->second.group - << " failed to resolve: "; - } - else - { - msg << std::endl << " one of the following failed to resolve: "; - } - bool first = true; - for ( const auto & [iid, descriptor] : group ) - { - if ( first ) { first = false; } - else { msg << ", "; } - msg << iid; - } - } - throw ResolutionFailure( msg.str() ); + ResolutionResult maybeResolved = this->tryResolveGroup( *group, system ); + std::visit( + overloaded { + /* Add to pkgs if the group was successfully resolved. */ + [&]( SystemPackages & resolved ) + { + pkgs.merge( resolved ); + group = groups.erase( group ); + }, + /* Otherwise add a description of the resolution failure to msg. */ + [&]( const ResolutionFailure & failure ) + { + /* We should only hit this on the first iteration. + * TODO: throw sooner rather than trying to resolve every + * group? */ + if ( failure.empty() ) + { + throw ResolutionFailureException( + "no inputs found to search for packages" ); + } + /* Print group name if this isn't the default group. */ + if ( auto descriptor = group->begin(); + descriptor != group->end() + && descriptor->second.group.has_value() ) + { + + msg << " in group `" << *descriptor->second.group << "': "; + } + else { msg << " in default group:"; } + msg << std::endl; + for ( const auto & [iid, url] : failure ) + { + msg << " failed to resolve `" << iid << "' in input `" << url + << "'"; + } + + ++group; + } }, + maybeResolved ); } + if ( ! groups.empty() ) { throw ResolutionFailureException( msg.str() ); } /* Copy over old lockfile entries we want to keep. */ if ( auto oldLockfile = this->getOldLockfile(); diff --git a/tests/environment.cc b/tests/environment.cc index 4e96a43b..405c6606 100644 --- a/tests/environment.cc +++ b/tests/environment.cc @@ -42,79 +42,95 @@ const System _system = "x86_64-linux"; /* -------------------------------------------------------------------------- */ -nlohmann::json helloLockedJSON { - { "input", - { { "fingerprint", nixpkgsFingerprintStr }, - { "url", nixpkgsRef }, - { "attrs", - { { "owner", "NixOS" }, - { "repo", "nixpkgs" }, - { "rev", nixpkgsRev }, - { "type", "github" }, - { "lastModified", 1685979279 }, - { "narHash", - "sha256-1UGacsv5coICyvAzwuq89v9NsS00Lo8sz22cDHwhnn8=" } } } } }, - { "attr-path", { "legacyPackages", _system, "hello" } }, - { "priority", 5 }, - { "info", - { { "broken", false }, - { "license", "GPL-3.0-or-later" }, - { "pname", "hello" }, - { "unfree", false }, - { "version", "2.12.1" } } } +nlohmann::json registryWithNixpkgsJSON { + { "inputs", + { { "nixpkgs", + { { "from", + { { "type", "github" }, + { "owner", "NixOS" }, + { "repo", "nixpkgs" }, + { "rev", nixpkgsRev } } }, + { "subtrees", { "legacyPackages" } } } } } } +}; +RegistryRaw registryWithNixpkgs( registryWithNixpkgsJSON ); + + +/* -------------------------------------------------------------------------- */ + +nlohmann::json inputWithNixpkgsJSON { + "input", + { { "fingerprint", nixpkgsFingerprintStr }, + { "url", nixpkgsRef }, + { "attrs", + { { "owner", "NixOS" }, + { "repo", "nixpkgs" }, + { "rev", nixpkgsRev }, + { "type", "github" }, + { "lastModified", 1685979279 }, + { "narHash", + "sha256-1UGacsv5coICyvAzwuq89v9NsS00Lo8sz22cDHwhnn8=" } } } } }; + + +/* -------------------------------------------------------------------------- */ + +nlohmann::json mockInputJSON { + "input", + { { "fingerprint", nixpkgsFingerprintStr }, + { "url", nixpkgsRef }, + { "attrs", + { { "owner", "owner" }, + { "repo", "repo" }, + { "rev", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" }, + { "type", "github" }, + { "lastModified", 1685979279 }, + { "narHash", + "sha256-1UGacsv5coICyvAzwuq89v9NsS00Lo8sz22cDHwhnn8=" } } } } +}; + + +/* -------------------------------------------------------------------------- */ + +nlohmann::json helloLockedJSON { inputWithNixpkgsJSON, + { "attr-path", + { "legacyPackages", _system, "hello" } }, + { "priority", 5 }, + { "info", + { { "broken", false }, + { "license", "GPL-3.0-or-later" }, + { "pname", "hello" }, + { "unfree", false }, + { "version", "2.12.1" } } } }; LockedPackageRaw helloLocked( helloLockedJSON ); /* -------------------------------------------------------------------------- */ /** Change a few fields from what we'd get if actual resultion was performed. */ -nlohmann::json mockHelloLockedJSON { - { "input", - { { "fingerprint", nixpkgsFingerprintStr }, - { "url", nixpkgsRef }, - { "attrs", - { { "owner", "owner" }, - { "repo", "repo" }, - { "rev", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" }, - { "type", "github" }, - { "lastModified", 1685979279 }, - { "narHash", - "sha256-1UGacsv5coICyvAzwuq89v9NsS00Lo8sz22cDHwhnn8=" } } } } }, - { "attr-path", { "mock", "hello" } }, - { "priority", 5 }, - { "info", - { { "broken", false }, - { "license", "GPL-3.0-or-later" }, - { "pname", "hello" }, - { "unfree", false }, - { "version", "2.12.1" } } } -}; +nlohmann::json mockHelloLockedJSON { mockInputJSON, + { "attr-path", { "mock", "hello" } }, + { "priority", 5 }, + { "info", + { { "broken", false }, + { "license", "GPL-3.0-or-later" }, + { "pname", "hello" }, + { "unfree", false }, + { "version", "2.12.1" } } } }; LockedPackageRaw mockHelloLocked( mockHelloLockedJSON ); /* -------------------------------------------------------------------------- */ -nlohmann::json curlLockedJSON - = { { "input", - { { "fingerprint", nixpkgsFingerprintStr }, - { "url", nixpkgsRef }, - { "attrs", - { { "owner", "NixOS" }, - { "repo", "nixpkgs" }, - { "rev", nixpkgsRev }, - { "type", "github" }, - { "lastModified", 1685979279 }, - { "narHash", - "sha256-1UGacsv5coICyvAzwuq89v9NsS00Lo8sz22cDHwhnn8=" } } } } }, - { "attr-path", { "legacyPackages", _system, "curl" } }, - { "priority", 5 }, - { "info", - { { "broken", false }, - { "license", "curl" }, - { "pname", "curl" }, - { "unfree", false }, - { "version", "8.1.1" } } } }; +nlohmann::json curlLockedJSON { inputWithNixpkgsJSON, + { "attr-path", + { "legacyPackages", _system, "curl" } }, + { "priority", 5 }, + { "info", + { { "broken", false }, + { "license", "curl" }, + { "pname", "curl" }, + { "unfree", false }, + { "version", "8.1.1" } } } }; LockedPackageRaw curlLocked( curlLockedJSON ); @@ -122,46 +138,18 @@ LockedPackageRaw curlLocked( curlLockedJSON ); /** Change a few fields from what we'd get if actual resultion was performed. */ -nlohmann::json mockCurlLockedJSON { - { "input", - { { "fingerprint", nixpkgsFingerprintStr }, - { "url", nixpkgsRef }, - { "attrs", - { { "owner", "owner" }, - { "repo", "repo" }, - { "rev", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" }, - { "type", "github" }, - { "lastModified", 1685979279 }, - { "narHash", - "sha256-1UGacsv5coICyvAzwuq89v9NsS00Lo8sz22cDHwhnn8=" } } } } }, - { "attr-path", { "mock", "curl" } }, - { "priority", 5 }, - { "info", - { { "broken", false }, - { "license", "GPL-3.0-or-later" }, - { "pname", "curl" }, - { "unfree", false }, - { "version", "2.12.1" } } } -}; +nlohmann::json mockCurlLockedJSON { mockInputJSON, + { "attr-path", { "mock", "curl" } }, + { "priority", 5 }, + { "info", + { { "broken", false }, + { "license", "GPL-3.0-or-later" }, + { "pname", "curl" }, + { "unfree", false }, + { "version", "2.12.1" } } } }; LockedPackageRaw mockCurlLocked( mockCurlLockedJSON ); -// /* -------------------------------------------------------------------------- -// */ - -nlohmann::json registryWithNixpkgsJSON { - { "inputs", - { { "nixpkgs", - { { "from", - { { "type", "github" }, - { "owner", "NixOS" }, - { "repo", "nixpkgs" }, - { "rev", nixpkgsRev } } }, - { "subtrees", { "legacyPackages" } } } } } } -}; -RegistryRaw registryWithNixpkgs( registryWithNixpkgsJSON ); - - /* -------------------------------------------------------------------------- */ bool @@ -882,6 +870,43 @@ test_createLockfile_both() } +/* -------------------------------------------------------------------------- */ + +/** @brief createLockfile gives a helpful error message when a package can't be + * resolved. */ +bool +test_createLockfile_error() +{ + /* Create manifest with hello */ + ManifestRaw manifestRaw; + manifestRaw.install = { { "not-a-package", std::nullopt } }; + manifestRaw.options = Options {}; + manifestRaw.options->systems = { _system }; + manifestRaw.registry = registryWithNixpkgs; + + EnvironmentManifest manifest( manifestRaw ); + + /* Test locking manifest creates expectedLockfile */ + Environment environment( std::nullopt, manifest, std::nullopt ); + try + { + Lockfile actualLockfile = environment.createLockfile(); + return false; + } + catch ( flox::FloxException & err ) + { + EXPECT_EQ( + err.what(), + std::string( + "resolution failure: failed to resolve some package(s):\n" + " in default group:\n" + " failed to resolve `not-a-package' in input " + "`github:NixOS/nixpkgs/e8039594435c68eb4f780f3e9bf3972a7399c4b1'" ) ); + return true; + } +} + + /* -------------------------------------------------------------------------- */ int @@ -908,6 +933,7 @@ main() RUN_TEST( createLockfile_new ); RUN_TEST( createLockfile_existing ); RUN_TEST( createLockfile_both ); + RUN_TEST( createLockfile_error ); return exitCode; } diff --git a/tests/test.hh b/tests/test.hh index 28e2abfd..7064f807 100644 --- a/tests/test.hh +++ b/tests/test.hh @@ -113,6 +113,10 @@ runTest( std::string_view name, F f, Args && ... args ) * * Assert that two expressions produce equal results, otherwise print them and * return `false'. + * + * Beware of comparing two char *. The following, for example, will fail: + * std::string foo( "foo" ); + * EXPECT_EQ( "foo", foo.c_str() ); */ #define EXPECT_EQ( EXPR_A, EXPR_B ) \ { \ From 261aeaf07e03a5b5a107e14a1be4472c5f37859b Mon Sep 17 00:00:00 2001 From: Matthew Kenigsberg Date: Mon, 20 Nov 2023 11:06:57 -0700 Subject: [PATCH 2/2] feat: test python module locking Test python and python modules are locked to a single input when they are in the same group. --- tests/environment.cc | 114 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/tests/environment.cc b/tests/environment.cc index 405c6606..8001d87b 100644 --- a/tests/environment.cc +++ b/tests/environment.cc @@ -150,6 +150,60 @@ nlohmann::json mockCurlLockedJSON { mockInputJSON, LockedPackageRaw mockCurlLocked( mockCurlLockedJSON ); +/* -------------------------------------------------------------------------- */ + +nlohmann::json pythonLockedJSON { + inputWithNixpkgsJSON, + { "attr-path", + // TODO this should be python310Packages not emacsPackages + { "legacyPackages", _system, "emacsPackages", "python" } }, + { "priority", 5 }, + { "info", + { { "broken", false }, + { "license", nullptr }, + { "pname", "python" }, + { "unfree", false }, + // TODO fix once python310Packages is used + { "version", "0.28" } } } +}; +LockedPackageRaw pythonLocked( pythonLockedJSON ); + + +/* -------------------------------------------------------------------------- */ + +nlohmann::json requestsLockedJSON { + inputWithNixpkgsJSON, + { "attr-path", + { "legacyPackages", _system, "python310Packages", "requests" } }, + { "priority", 5 }, + { "info", + { { "broken", false }, + { "license", "Apache-2.0" }, + { "pname", "requests" }, + { "unfree", false }, + { "version", "2.31.0" } } } +}; +LockedPackageRaw requestsLocked( requestsLockedJSON ); + + +/* -------------------------------------------------------------------------- */ + +nlohmann::json pandasLockedJSON { + inputWithNixpkgsJSON, + { "attr-path", { "legacyPackages", _system, "python310Packages", "pandas" } }, + { "priority", 5 }, + { "info", + { { "broken", false }, + { "license", "BSD-3-Clause" }, + { "pname", "pandas" }, + { "unfree", false }, + { "version", "1.5.3" } } } +}; +LockedPackageRaw pandasLocked( pandasLockedJSON ); + + + + /* -------------------------------------------------------------------------- */ bool @@ -782,6 +836,65 @@ test_createLockfile_new() } +/* -------------------------------------------------------------------------- */ + +/** + * @brief createLockfile locks python and python modules to a single input when + * they are in the same group. */ +bool +test_createLockfile_new_python_modules() +{ + /* Create manifest with python, requests, and pandas in the same group. */ + ManifestRaw manifestRaw; + /* If we change the default grouping strategy, we'll need to add an explicit + * group. */ + manifestRaw.install = { { "python", std::nullopt }, + { "requests", std::nullopt }, + { "pandas", std::nullopt } }; + manifestRaw.options = Options {}; + manifestRaw.options->systems = { _system }; + manifestRaw.registry = registryWithNixpkgs; + + EnvironmentManifest manifest( manifestRaw ); + + /* Create expected lockfile, reusing manifestRaw */ + LockfileRaw expectedLockfileRaw; + expectedLockfileRaw.packages = { { _system, + { { "python", pythonLocked }, + { "requests", requestsLocked }, + { "pandas", pandasLocked } } } }; + expectedLockfileRaw.manifest = manifestRaw; + + Lockfile expectedLockfile( expectedLockfileRaw ); + + /* Test locking manifest creates expectedLockfile, and the input for all 3 + * packages is the same. */ + Environment environment( std::nullopt, manifest, std::nullopt ); + Lockfile actualLockfile = environment.createLockfile(); + /* We could just check the inputs, but might as well check the whole lockfile. + */ + EXPECT( equalLockfile( actualLockfile, expectedLockfile ) ); + EXPECT( equalLockedInputRaw( actualLockfile.getLockfileRaw() + .packages.at( _system ) + .at( "python" ) + ->input, + actualLockfile.getLockfileRaw() + .packages.at( _system ) + .at( "requests" ) + ->input ) ); + EXPECT( equalLockedInputRaw( actualLockfile.getLockfileRaw() + .packages.at( _system ) + .at( "requests" ) + ->input, + actualLockfile.getLockfileRaw() + .packages.at( _system ) + .at( "pandas" ) + ->input ) ); + + return true; +} + + /* -------------------------------------------------------------------------- */ /** @brief `createLockfile()` reuses existing lockfile entry. */ @@ -931,6 +1044,7 @@ main() RUN_TEST( getGroupInput3 ); RUN_TEST( createLockfile_new ); + RUN_TEST( createLockfile_new_python_modules ); RUN_TEST( createLockfile_existing ); RUN_TEST( createLockfile_both ); RUN_TEST( createLockfile_error );