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 0000000..09603f4 Binary files /dev/null and b/tst/data/new/pmdummy/doc/manual.pdf differ diff --git a/tst/data/new/pmdummy/doc/manual.six b/tst/data/new/pmdummy/doc/manual.six new file mode 100644 index 0000000..ca33fed --- /dev/null +++ b/tst/data/new/pmdummy/doc/manual.six @@ -0,0 +1,2 @@ +#SIXFILE +# Not a real manual.six; a placeholder so that ValidatePackageInfo is happy. diff --git a/tst/data/new/pmdummy/init.g b/tst/data/new/pmdummy/init.g new file mode 100644 index 0000000..3bd9844 --- /dev/null +++ b/tst/data/new/pmdummy/init.g @@ -0,0 +1,3 @@ +# +# pmdummy: a dummy package for the PackageManager tests +# diff --git a/tst/data/new/pmdummy/read.g b/tst/data/new/pmdummy/read.g new file mode 100644 index 0000000..3bd9844 --- /dev/null +++ b/tst/data/new/pmdummy/read.g @@ -0,0 +1,3 @@ +# +# pmdummy: a dummy package for the PackageManager tests +# diff --git a/tst/data/old/pmdummy/PackageInfo.g b/tst/data/old/pmdummy/PackageInfo.g new file mode 100644 index 0000000..e83b250 --- /dev/null +++ b/tst/data/old/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 := "1.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-1.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/old/pmdummy/README.md b/tst/data/old/pmdummy/README.md new file mode 100644 index 0000000..c1f45d9 --- /dev/null +++ b/tst/data/old/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/old/pmdummy/doc/chap0.html b/tst/data/old/pmdummy/doc/chap0.html new file mode 100644 index 0000000..8de2db8 --- /dev/null +++ b/tst/data/old/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/old/pmdummy/doc/manual.pdf b/tst/data/old/pmdummy/doc/manual.pdf new file mode 100644 index 0000000..09603f4 Binary files /dev/null and b/tst/data/old/pmdummy/doc/manual.pdf differ diff --git a/tst/data/old/pmdummy/doc/manual.six b/tst/data/old/pmdummy/doc/manual.six new file mode 100644 index 0000000..ca33fed --- /dev/null +++ b/tst/data/old/pmdummy/doc/manual.six @@ -0,0 +1,2 @@ +#SIXFILE +# Not a real manual.six; a placeholder so that ValidatePackageInfo is happy. diff --git a/tst/data/old/pmdummy/init.g b/tst/data/old/pmdummy/init.g new file mode 100644 index 0000000..3bd9844 --- /dev/null +++ b/tst/data/old/pmdummy/init.g @@ -0,0 +1,3 @@ +# +# pmdummy: a dummy package for the PackageManager tests +# diff --git a/tst/data/old/pmdummy/read.g b/tst/data/old/pmdummy/read.g new file mode 100644 index 0000000..3bd9844 --- /dev/null +++ b/tst/data/old/pmdummy/read.g @@ -0,0 +1,3 @@ +# +# pmdummy: a dummy package for the PackageManager tests +# diff --git a/tst/data/pkglist.csv b/tst/data/pkglist.csv new file mode 100644 index 0000000..d4b9c51 --- /dev/null +++ b/tst/data/pkglist.csv @@ -0,0 +1,2 @@ +# A package URLs list, in the format used by the GAP package distribution. +pmdummy http://@SERVER@/PackageInfo.g diff --git a/tst/distro.tst b/tst/distro.tst index e3ba0df..b9ba328 100644 --- a/tst/distro.tst +++ b/tst/distro.tst @@ -76,18 +76,30 @@ gap> 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;