From 780e98c61c49ed2a0a2836ec244b589af6501ef3 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Sat, 1 Aug 2026 00:33:01 +0200 Subject: [PATCH] Run the download tests against a local HTTP server Several tests depended on third-party web servers being reachable, which made them fail spuriously: * The tests for the two failure paths of GetPackageURLs pointed PKGMAN_PackageInfoURLList at https://www.gap-system.org, purely to get back some content that is not a valid package URLs list. Whenever that single self-hosted server was unreachable, the test took the "Could not contact server" branch instead and failed. This was the cause of every recent spurious "packagemanager" failure in the PackageDistro CI. It was fragile in a second way too, since the expected output contained the first 71 bytes of that page's HTML. * The test for updating a package whose directory name does not contain a version number installed transgrp 3.6.4 from a personal university web page and then updated it to 3.6.5 -- 118MB downloaded from a third-party server on every single test run. Instead start a small local HTTP server for these tests, cribbed from the utils package, serving a document root prepared from the new tst/data directory. Files that need to refer back to the server contain an "@SERVER@" placeholder, since the port is only known at runtime. The bad package URLs list is now simply a file there. For the update test, tst/data holds the sources of two versions of a minimal dummy package "pmdummy", which are packed into tarballs when the tests run, so that nothing binary needs to be checked in and their contents stay reviewable; both unpack into a directory without a version number, which is the situation under test. For "Could not contact server" we use an unused loopback port rather than an unresponsive server, since wget retries the latter many times. Also report the underlying error of a failed download at info level 2, so that transient network problems can be diagnosed after the fact. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGES | 8 ++ PackageInfo.g | 1 + gap/distro.gi | 2 + gap/download.gd | 1 + gap/download.gi | 24 +++- tst/archive.tst | 30 +++-- tst/data/badurls.txt | 2 + tst/data/new/pmdummy/PackageInfo.g | 55 +++++++++ tst/data/new/pmdummy/README.md | 4 + tst/data/new/pmdummy/doc/chap0.html | 3 + tst/data/new/pmdummy/doc/manual.pdf | Bin 0 -> 79 bytes tst/data/new/pmdummy/doc/manual.six | 2 + tst/data/new/pmdummy/init.g | 3 + tst/data/new/pmdummy/read.g | 3 + tst/data/old/pmdummy/PackageInfo.g | 55 +++++++++ tst/data/old/pmdummy/README.md | 4 + tst/data/old/pmdummy/doc/chap0.html | 3 + tst/data/old/pmdummy/doc/manual.pdf | Bin 0 -> 79 bytes tst/data/old/pmdummy/doc/manual.six | 2 + tst/data/old/pmdummy/init.g | 3 + tst/data/old/pmdummy/read.g | 3 + tst/data/pkglist.csv | 2 + tst/distro.tst | 18 ++- tst/http-server.g | 183 ++++++++++++++++++++++++++++ 24 files changed, 397 insertions(+), 14 deletions(-) create mode 100644 tst/data/badurls.txt create mode 100644 tst/data/new/pmdummy/PackageInfo.g create mode 100644 tst/data/new/pmdummy/README.md create mode 100644 tst/data/new/pmdummy/doc/chap0.html create mode 100644 tst/data/new/pmdummy/doc/manual.pdf create mode 100644 tst/data/new/pmdummy/doc/manual.six create mode 100644 tst/data/new/pmdummy/init.g create mode 100644 tst/data/new/pmdummy/read.g create mode 100644 tst/data/old/pmdummy/PackageInfo.g create mode 100644 tst/data/old/pmdummy/README.md create mode 100644 tst/data/old/pmdummy/doc/chap0.html create mode 100644 tst/data/old/pmdummy/doc/manual.pdf create mode 100644 tst/data/old/pmdummy/doc/manual.six create mode 100644 tst/data/old/pmdummy/init.g create mode 100644 tst/data/old/pmdummy/read.g create mode 100644 tst/data/pkglist.csv create mode 100644 tst/http-server.g diff --git a/CHANGES b/CHANGES index d3e7398..f8be30b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,5 +1,13 @@ This file describes changes in the PackageManager package. +unreleased + - Report the underlying error when a download fails (at InfoPackageManager + level 2) + - Test the GetPackageURLs failure paths against a local HTTP server instead + of a live website, which was a frequent source of spurious test failures + - Test updating a package installed in a directory without a version number + against the same local server, instead of downloading transgrp twice + 1.6.3 (2025-05-14) - Alter tests for compatibility with AutoDoc 2025.05.09 diff --git a/PackageInfo.g b/PackageInfo.g index 238adcd..cb2f415 100644 --- a/PackageInfo.g +++ b/PackageInfo.g @@ -87,6 +87,7 @@ Dependencies := rec( SuggestedOtherPackages := [ [ "GAPDoc", ">= 1.6.1" ], [ "AutoDoc", ">= 2025.05.09" ], [ "curlInterface", ">= 2.1.0" ] ], + TestPackages := [ [ "io", ">= 4.7.0" ] ], ExternalConditions := [ ], ), diff --git a/gap/distro.gi b/gap/distro.gi index dff04a8..24ef433 100644 --- a/gap/distro.gi +++ b/gap/distro.gi @@ -111,6 +111,8 @@ function() urls := rec(success := false); if not get.success then Info(InfoPackageManager, 1, "Could not contact server"); + Info(InfoPackageManager, 2, "Tried to download ", PKGMAN_PackageInfoURLList); + PKGMAN_InfoDownloadError(get); return urls; fi; for line in SplitString(get.result, "\n") do diff --git a/gap/download.gd b/gap/download.gd index 10b68f4..0510c2e 100644 --- a/gap/download.gd +++ b/gap/download.gd @@ -1,3 +1,4 @@ +DeclareGlobalFunction("PKGMAN_InfoDownloadError"); DeclareGlobalFunction("PKGMAN_DownloadUrlToTempFile"); DeclareGlobalFunction("PKGMAN_DownloadURL"); DeclareGlobalFunction("PKGMAN_DownloadPackageInfo"); diff --git a/gap/download.gi b/gap/download.gi index 66499b1..32a0f18 100644 --- a/gap/download.gi +++ b/gap/download.gi @@ -1,3 +1,13 @@ +# Report why a download failed. This is only shown at info level 2, so that +# the (tested) info level 1 output stays stable, but it makes transient +# network problems diagnosable after the fact, e.g. in CI logs. +InstallGlobalFunction(PKGMAN_InfoDownloadError, +function(get) + if IsRecord(get) and IsBound(get.error) then + Info(InfoPackageManager, 2, "Download error: ", get.error); + fi; +end); + InstallGlobalFunction(PKGMAN_DownloadUrlToTempFile, function(url) local get, url_parts, filename, path; @@ -5,6 +15,7 @@ function(url) get := PKGMAN_DownloadURL(url); if get.success <> true then Info(InfoPackageManager, 1, "Could not download from ", url); + PKGMAN_InfoDownloadError(get); return fail; fi; url_parts := SplitString(url, "/"); @@ -18,7 +29,7 @@ end); InstallGlobalFunction(PKGMAN_DownloadURL, function(url) - local tool, exec; + local tool, exec, errors; # Use curlInterface if available if TestPackageAvailability("curlInterface", PKGMAN_CurlIntReqVer) = true then @@ -27,19 +38,27 @@ function(url) fi; # Try command line tools (wget/curl) + errors := []; for tool in PKGMAN_DownloadCmds do Info(InfoPackageManager, 4, "Using ", tool[1], " to download..."); exec := CallFuncList(PKGMAN_Exec, Concatenation(["."], [tool[1]], tool[2], [url])); if exec = fail then Info(InfoPackageManager, 4, tool[1], " unavailable"); + Add(errors, Concatenation(tool[1], " unavailable")); elif exec.code <> 0 then Info(InfoPackageManager, 4, "Download failed with ", tool[1]); + Add(errors, Concatenation(tool[1], " failed with exit code ", + String(exec.code))); else return rec(success := true, result := exec.output); fi; od; - return rec(success := false, error := "no download method is available"); + if IsEmpty(errors) then + return rec(success := false, error := "no download method is available"); + fi; + return rec(success := false, + error := JoinStringsWithSeparator(errors, "; ")); end); InstallGlobalFunction(PKGMAN_DownloadPackageInfo, @@ -50,6 +69,7 @@ function(url) get := PKGMAN_DownloadURL(url); if not get.success then Info(InfoPackageManager, 1, "Unable to download from ", url); + PKGMAN_InfoDownloadError(get); return fail; fi; info := PKGMAN_GetPackageInfo(InputTextString(get.result)); diff --git a/tst/archive.tst b/tst/archive.tst index 0c88571..8de9a2f 100644 --- a/tst/archive.tst +++ b/tst/archive.tst @@ -21,23 +21,35 @@ gap> InstallPackage("https://gap-packages.github.io/PackageManager/dummy/badpack #I PackageInfo.g lacks PackageName field false -# Updating old package that doesn't have the version number in its directory name -gap> InstallPackage("https://www.math.colostate.edu/~hulpke/transgrp/transgrp3.6.4.tar.gz"); +# Updating old package that doesn't have the version number in its directory +# name. We use a dummy package served by a local HTTP server: the real-world +# example used to be transgrp, but downloading it twice meant fetching 120MB +# from a third-party server on every test run. +gap> LoadPackage("io", false); true -gap> oldinfo := First(PackageInfo("transgrp"), x -> x.Version = "3.6.4");; +gap> ReadPackage("PackageManager", "tst/http-server.g"); +true +gap> server := PKGMAN_StartHTTPTestServer(PKGMAN_PrepareTestData());; +gap> InstallPackage(Concatenation(server.url, "/pmdummy-1.0.tar.gz")); +true +gap> oldinfo := First(PackageInfo("pmdummy"), x -> x.Version = "1.0");; gap> oldinfo <> fail; true -gap> PositionSublist(oldinfo.InstallationPath, "3.6.4"); # version number not in dir name +gap> PositionSublist(oldinfo.InstallationPath, "1.0"); # version number not in dir name fail -gap> UpdatePackage("transgrp", false); # also removes old version +gap> urllist := PKGMAN_PackageInfoURLList;; +gap> PKGMAN_PackageInfoURLList := Concatenation(server.url, "/pkglist.csv");; +gap> UpdatePackage("pmdummy", false); # also removes old version #I Package already installed at target location #I Appending '.old' to old version directory true -gap> newinfo := PKGMAN_UserPackageInfo("transgrp")[1];; -gap> CompareVersionNumbers(newinfo.Version, ">=3.6.5"); -true -gap> RemovePackage("transgrp", false); +gap> PKGMAN_PackageInfoURLList := urllist;; +gap> newinfo := PKGMAN_UserPackageInfo("pmdummy")[1];; +gap> newinfo.Version; +"2.0" +gap> RemovePackage("pmdummy", false); true +gap> PKGMAN_StopHTTPTestServer(server); # Install to existing empty directory gap> CreateDir(Filename(Directory(PKGMAN_PackageDir()), "Toric-1.9.5")); diff --git a/tst/data/badurls.txt b/tst/data/badurls.txt new file mode 100644 index 0000000..c780334 --- /dev/null +++ b/tst/data/badurls.txt @@ -0,0 +1,2 @@ +# comment lines are ignored + diff --git a/tst/data/new/pmdummy/PackageInfo.g b/tst/data/new/pmdummy/PackageInfo.g new file mode 100644 index 0000000..e0aefa7 --- /dev/null +++ b/tst/data/new/pmdummy/PackageInfo.g @@ -0,0 +1,55 @@ +# +# A minimal dummy package for the PackageManager tests. It is served by the +# test HTTP server (see tst/http-server.g); "@SERVER@" is replaced by the +# address of that server when the file is served, but not inside the tarball. +# +# Note that the tarball of this package unpacks into a directory whose name +# does not contain the version number, which is what we want to test. +# +SetPackageInfo( rec( + +PackageName := "pmdummy", +Subtitle := "A dummy package for the PackageManager tests", +Version := "2.0", +Date := "01/01/2020", # dd/mm/yyyy format +License := "GPL-2.0-or-later", + +Persons := [ + rec( + IsAuthor := true, + IsMaintainer := true, + FirstNames := "The GAP", + LastName := "Team", + Email := "support@gap-system.org", + ), +], + +PackageWWWHome := "http://@SERVER@/", +ArchiveURL := "http://@SERVER@/pmdummy-2.0", +README_URL := "http://@SERVER@/README.md", +PackageInfoURL := "http://@SERVER@/PackageInfo.g", +ArchiveFormats := ".tar.gz", + +Status := "other", + +AbstractHTML := "A dummy package for the PackageManager tests", + +PackageDoc := rec( + BookName := "pmdummy", + ArchiveURLSubset := ["doc"], + HTMLStart := "doc/chap0.html", + PDFFile := "doc/manual.pdf", + SixFile := "doc/manual.six", + LongTitle := "A dummy package for the PackageManager tests", +), + +Dependencies := rec( + GAP := ">= 4.12", + NeededOtherPackages := [], + SuggestedOtherPackages := [], + ExternalConditions := [], +), + +AvailabilityTest := ReturnTrue, + +)); diff --git a/tst/data/new/pmdummy/README.md b/tst/data/new/pmdummy/README.md new file mode 100644 index 0000000..c1f45d9 --- /dev/null +++ b/tst/data/new/pmdummy/README.md @@ -0,0 +1,4 @@ +# pmdummy + +A minimal dummy package used by the PackageManager tests. It is never +loaded; it only needs to be installable. diff --git a/tst/data/new/pmdummy/doc/chap0.html b/tst/data/new/pmdummy/doc/chap0.html new file mode 100644 index 0000000..8de2db8 --- /dev/null +++ b/tst/data/new/pmdummy/doc/chap0.html @@ -0,0 +1,3 @@ +pmdummy +Dummy manual, so that the tests need not build any documentation. + diff --git a/tst/data/new/pmdummy/doc/manual.pdf b/tst/data/new/pmdummy/doc/manual.pdf new file mode 100644 index 0000000000000000000000000000000000000000..09603f44f1f52398f6144d70e14be99104ac4b05 GIT binary patch literal 79 zcmY!laBpmdummy +Dummy manual, so that the tests need not build any documentation. + diff --git a/tst/data/old/pmdummy/doc/manual.pdf b/tst/data/old/pmdummy/doc/manual.pdf new file mode 100644 index 0000000000000000000000000000000000000000..09603f44f1f52398f6144d70e14be99104ac4b05 GIT binary patch literal 79 zcmY!laB InstallRequiredPackages(); false gap> GAPInfo.Dependencies := rec(NeededOtherPackages := backup);; -# GetPackageURLs failure +# GetPackageURLs failures. These use a local HTTP server rather than any +# remote one, since otherwise a transient network problem makes the "server +# unreachable" case indistinguishable from the "bad content" case. +gap> LoadPackage("io", false); +true +gap> ReadPackage("PackageManager", "tst/http-server.g"); +true +gap> server := PKGMAN_StartHTTPTestServer(PKGMAN_PrepareTestData());; gap> default_url := PKGMAN_PackageInfoURLList;; -gap> PKGMAN_PackageInfoURLList := "http://www.nothing.rubbish/abc.txt";; + +# The server cannot be contacted (nothing is listening on that port) +gap> PKGMAN_PackageInfoURLList := PKGMAN_UnusedURL();; gap> GetPackageURLs(); #I Could not contact server rec( success := false ) -gap> PKGMAN_PackageInfoURLList := "https://www.gap-system.org";; + +# The server answers, but with something that is not a package URLs list +gap> PKGMAN_PackageInfoURLList := Concatenation(server.url, "/badurls.txt");; gap> GetPackageURLs(); #I Bad line in package URLs list: #I PKGMAN_PackageInfoURLList := default_url;; +gap> PKGMAN_StopHTTPTestServer(server); # InstallPackageFromName failure gap> InstallPackage("sillypackage"); diff --git a/tst/http-server.g b/tst/http-server.g new file mode 100644 index 0000000..6c06d10 --- /dev/null +++ b/tst/http-server.g @@ -0,0 +1,183 @@ +############################################################################# +## +## A small HTTP server for the tests, so that they do not depend on any +## remote server being reachable. Requires the IO package. +## +## Adapted from the equivalent file in the utils package. +## +## The server hands out the files in a document root directory, which is +## prepared by PKGMAN_PrepareTestData from the contents of tst/data. Since +## the server listens on an arbitrary free port, files that need to refer to +## the server itself contain the placeholder "@SERVER@", which is replaced by +## the actual address when such a file is served (but not when it is packed +## into a tarball). +## + +## Note that we assign the globals below instead of using BindGlobal, since +## several test files read this file in the same GAP session. + +# Set by PKGMAN_StartHTTPTestServer before forking, so that the forked +# processes inherit them. +PKGMAN_HTTPTestRoot := fail; +PKGMAN_HTTPTestAddress := fail; + +PKGMAN_HandleHTTPTestRequest := function(listener, socket) + local connection, line, parts, name, pos, body, status; + + IO_close(listener); + connection := IO_WrapFD(socket, IO.DefaultBufSize, IO.DefaultBufSize); + line := IO_ReadLine(connection); + parts := SplitString(line, " \r\n"); + if Length(parts) < 2 then + IO_Close(connection); + IO_exit(1); + fi; + + # Skip the request headers + repeat + line := IO_ReadLine(connection); + until line = fail or line = "" or line = "\n" or line = "\r\n"; + + # We serve a flat directory only, so the file name must be a plain name + name := parts[2]; + pos := Position(name, '?'); + if pos <> fail then + name := name{[1 .. pos - 1]}; + fi; + if StartsWith(name, "/") then + name := name{[2 .. Length(name)]}; + fi; + if name = "" or '/' in name or name = ".." then + body := fail; + else + body := StringFile(Filename(Directory(PKGMAN_HTTPTestRoot), name)); + fi; + + if body = fail then + status := "404 Not Found"; + body := "no such file\n"; + else + status := "200 OK"; + if not ForAny([".tar.gz", ".tar.bz2"], ext -> EndsWith(name, ext)) then + body := ReplacedString(body, "@SERVER@", PKGMAN_HTTPTestAddress); + fi; + fi; + + IO_Write(connection, + "HTTP/1.1 ", status, "\r\n", + "Content-Type: application/octet-stream\r\n", + "Content-Length: ", String(Length(body)), "\r\n", + "Connection: close\r\n\r\n", + body); + IO_Flush(connection); + IO_Close(connection); + IO_exit(0); +end; + +# Start the server in a forked process, serving the files in the directory +# on an ephemeral port of the loopback interface. Returns a record +# with components 'pid' and 'url', the latter being the base URL. +PKGMAN_StartHTTPTestServer := function(root) + local listener, address, port, pid, socket, handler; + + listener := IO_socket(IO.PF_INET, IO.SOCK_STREAM, "tcp"); + if listener = fail or + IO_bind(listener, IO_MakeIPAddressPort("127.0.0.1", 0)) = fail or + IO_listen(listener, 8) <> true then + Error("cannot start the HTTP test server"); + fi; + address := IO_getsockname(listener); + port := 256 * INT_CHAR(address[3]) + INT_CHAR(address[4]); + + PKGMAN_HTTPTestRoot := root; + PKGMAN_HTTPTestAddress := Concatenation("127.0.0.1:", String(port)); + + pid := IO_fork(); + if pid = 0 then + while true do + socket := IO_accept(listener, IO_MakeIPAddressPort("0.0.0.0", 0)); + if socket = fail then + IO_exit(0); + fi; + handler := IO_fork(); + if handler = 0 then + PKGMAN_HandleHTTPTestRequest(listener, socket); + elif handler < 0 then + IO_close(socket); + IO_exit(1); + else + IO_close(socket); + IO_IgnorePid(handler); + fi; + od; + elif pid < 0 then + IO_close(listener); + Error("cannot fork the HTTP test server"); + fi; + + IO_close(listener); + return rec(pid := pid, + url := Concatenation("http://", PKGMAN_HTTPTestAddress)); +end; + +PKGMAN_StopHTTPTestServer := function(server) + IO_kill(server.pid, IO.SIGTERM); + IO_WaitPid(server.pid, true); +end; + +# A URL on the loopback interface with nothing listening on it, so that +# connecting to it fails immediately. We ask the kernel for a free port and +# then release it again; note that we deliberately do not simulate an +# unresponsive server instead, since wget would retry that many times. +PKGMAN_UnusedURL := function() + local socket, address, port; + + socket := IO_socket(IO.PF_INET, IO.SOCK_STREAM, "tcp"); + if socket = fail or + IO_bind(socket, IO_MakeIPAddressPort("127.0.0.1", 0)) = fail then + Error("cannot find an unused port"); + fi; + address := IO_getsockname(socket); + port := 256 * INT_CHAR(address[3]) + INT_CHAR(address[4]); + IO_close(socket); + return Concatenation("http://127.0.0.1:", String(port)); +end; + +# Assemble a document root for the test server in a temporary directory: the +# plain files of tst/data, plus a tarball of each dummy package version. Note +# that the tarballs are built here rather than checked in, so that their +# contents stay reviewable. +PKGMAN_PrepareTestData := function() + local data, root, name, exec; + + data := Filename(DirectoriesPackageLibrary("PackageManager", "tst"), "data"); + root := Filename(DirectoryTemporary(), ""); + + for name in ["badurls.txt", "pkglist.csv"] do + exec := PKGMAN_Exec(".", "cp", Filename(Directory(data), name), root); + if exec.code <> 0 then + Error("cannot copy the test data"); + fi; + od; + + # The PackageInfo.g of the newest version, as the distribution serves it + exec := PKGMAN_Exec(".", "cp", + Filename(Directory(data), "new/pmdummy/PackageInfo.g"), + root); + if exec.code <> 0 then + Error("cannot copy the test data"); + fi; + + # Both tarballs unpack into a directory "pmdummy" without a version number + for name in [["old", "1.0"], ["new", "2.0"]] do + exec := PKGMAN_Exec(".", "tar", "-czf", + Filename(Directory(root), + Concatenation("pmdummy-", name[2], ".tar.gz")), + "-C", Filename(Directory(data), name[1]), "pmdummy"); + if exec.code <> 0 then + Error("cannot build the dummy package tarballs"); + fi; + od; + + return root; +end;