diff --git a/bash/patchman-client b/bash/patchman-client index a1f257e..80b3ee9 100755 --- a/bash/patchman-client +++ b/bash/patchman-client @@ -591,6 +591,7 @@ get_repos() { mirror_file=$(echo "${mirror_repo}" | sed -e "s/.* mirror+file://g" | cut -d " " -f 1) if [ -f "${mirror_file}" ] ; then for url in $(cat ${mirror_file}) ; do + url=${url%/} dist_repo=$(echo "${mirror_repo}" | sed -e "s#mirror+file:${mirror_file}#${url}#g") dist_repos="${dist_repos}"$'\n'"${dist_repo}" done diff --git a/go/lib/repos/apt.go b/go/lib/repos/apt.go index 47151c2..a05e8c8 100644 --- a/go/lib/repos/apt.go +++ b/go/lib/repos/apt.go @@ -37,16 +37,19 @@ func PrintAptReposTable(repos []AptRepo) { } func ConstructAptRepoURL(url string, components []string) string { - repo_url := url - if components[0] != "" { - repo_url += "/dists/" + components[0] - } - if components[1] != "Packages" { - repo_url += "/binary-" + components[1] + repo_url := strings.TrimRight(url, "/") + if len(components) < 2 || components[1] == "Packages" { + // flat repo, e.g. "./ Packages" + if len(components) > 0 { + repo_url += "/" + components[0] + } + return repo_url } - return repo_url + return repo_url + "/dists/" + components[0] + "/binary-" + components[1] } +var aptRepoLineRe = regexp.MustCompile(`^\s*-?[0-9]+\s+(mirror\+file:|https?:)`) + func GetAptRepos(os_info si.OSInfo) ([]AptRepo, error) { cmd, err := util.RunCommand("apt-cache", "policy") if err != nil { @@ -55,21 +58,27 @@ func GetAptRepos(os_info si.OSInfo) ([]AptRepo, error) { if cmd == nil { return nil, nil // apt-cache not available } + return parseAptPolicy(string(cmd), os_info), nil +} +func parseAptPolicy(output string, os_info si.OSInfo) []AptRepo { apt_repos := []AptRepo{} - repoLineRe := regexp.MustCompile(`^ *[0-9]{1,5} mirror\+file|http(s)?:`) - for _, line := range strings.Split(string(cmd), "\n") { + for _, line := range strings.Split(output, "\n") { if strings.Contains(line, "Translation") { continue } - if !repoLineRe.MatchString(line) { + if !aptRepoLineRe.MatchString(line) { continue } - v := strings.Split(line, " ") - priority := v[1] + // priority is right-aligned, so the number of leading spaces varies + v := strings.Fields(line) + if len(v) < 4 { + continue + } + priority := v[0] repo_urls := []string{} - if strings.HasPrefix(v[2], "mirror+file:/") { - mirror_file := strings.Replace(v[2], "mirror+file:", "", 1) + if strings.HasPrefix(v[1], "mirror+file:/") { + mirror_file := strings.Replace(v[1], "mirror+file:", "", 1) content, err := os.ReadFile(mirror_file) if err != nil { slog.Warn("failed to read mirror file", "file", mirror_file, "error", err) @@ -77,12 +86,15 @@ func GetAptRepos(os_info si.OSInfo) ([]AptRepo, error) { } for _, mirror := range strings.Split(string(content), "\n") { if mirror != "" { - repo_url := ConstructAptRepoURL(mirror, v[3:]) + repo_url := ConstructAptRepoURL(mirror, v[2:]) repo_urls = append(repo_urls, repo_url) } } + if len(repo_urls) == 0 { + continue + } } else { - repo_url := ConstructAptRepoURL(v[2], v[3:]) + repo_url := ConstructAptRepoURL(v[1], v[2:]) repo_urls = append(repo_urls, repo_url) } name := fmt.Sprintf("%s %s %s repo at %s", os_info.OSVendor, os_info.OSRelease, os_info.OSArch, repo_urls[0]) @@ -92,8 +104,7 @@ func GetAptRepos(os_info si.OSInfo) ([]AptRepo, error) { Urls: repo_urls, }) } - - return apt_repos, nil + return apt_repos } func PrintAptRepos(repos []AptRepo) { diff --git a/go/lib/repos/repos_test.go b/go/lib/repos/repos_test.go index 7674e3c..93c0f92 100644 --- a/go/lib/repos/repos_test.go +++ b/go/lib/repos/repos_test.go @@ -2,8 +2,11 @@ package repos import ( "os" + "reflect" "strings" "testing" + + "github.com/furlongm/patchman-client/go/lib/si" ) func TestGetYumRepos(t *testing.T) { @@ -222,6 +225,80 @@ enabled=1 } } +func TestConstructAptRepoURL(t *testing.T) { + tests := []struct { + url string + components []string + want string + }{ + {"http://deb.debian.org/debian", []string{"bookworm/main", "amd64", "Packages"}, + "http://deb.debian.org/debian/dists/bookworm/main/binary-amd64"}, + {"http://deb.debian.org/debian/", []string{"bookworm/main", "amd64", "Packages"}, + "http://deb.debian.org/debian/dists/bookworm/main/binary-amd64"}, + {"https://repo.example.com/flat", []string{"./", "Packages"}, + "https://repo.example.com/flat/./"}, + } + for _, tt := range tests { + if got := ConstructAptRepoURL(tt.url, tt.components); got != tt.want { + t.Errorf("ConstructAptRepoURL(%q, %q) = %q, want %q", tt.url, tt.components, got, tt.want) + } + } +} + +func TestParseAptPolicy(t *testing.T) { + output := `Package files: + 100 /var/lib/dpkg/status + release a=now + 500 http://deb.debian.org/debian bookworm/main amd64 Packages + release v=12,o=Debian,a=stable,n=bookworm,l=Debian,c=main,b=amd64 + origin deb.debian.org + 500 http://deb.debian.org/debian bookworm/main Translation-en + 1 http://deb.debian.org/debian experimental/main amd64 Packages +1001 http://apt.example.com/pinned stable/main amd64 Packages + -10 http://apt.example.com/never stable/main amd64 Packages + 500 https://repo.example.com/flat ./ Packages +Pinned packages: +` + want := []AptRepo{ + {Priority: "500", Urls: []string{"http://deb.debian.org/debian/dists/bookworm/main/binary-amd64"}}, + {Priority: "1", Urls: []string{"http://deb.debian.org/debian/dists/experimental/main/binary-amd64"}}, + {Priority: "1001", Urls: []string{"http://apt.example.com/pinned/dists/stable/main/binary-amd64"}}, + {Priority: "-10", Urls: []string{"http://apt.example.com/never/dists/stable/main/binary-amd64"}}, + {Priority: "500", Urls: []string{"https://repo.example.com/flat/./"}}, + } + got := parseAptPolicy(output, si.OSInfo{}) + if len(got) != len(want) { + t.Fatalf("got %d repos, want %d: %+v", len(got), len(want), got) + } + for i := range want { + if got[i].Priority != want[i].Priority || !reflect.DeepEqual(got[i].Urls, want[i].Urls) { + t.Errorf("repo %d: got %s %q, want %s %q", i, got[i].Priority, got[i].Urls, want[i].Priority, want[i].Urls) + } + } +} + +func TestParseAptPolicyMirrorFile(t *testing.T) { + dir := t.TempDir() + mirrors := dir + "/mirrors.txt" + if err := os.WriteFile(mirrors, []byte("http://a.example.com/debian/\nhttps://b.example.com/debian\n"), 0o644); err != nil { + t.Fatal(err) + } + empty := dir + "/empty.txt" + if err := os.WriteFile(empty, []byte("\n"), 0o644); err != nil { + t.Fatal(err) + } + output := " 500 mirror+file:" + mirrors + " bookworm/main amd64 Packages\n" + + " 500 mirror+file:" + empty + " bookworm/main amd64 Packages\n" + got := parseAptPolicy(output, si.OSInfo{}) + want := []string{ + "http://a.example.com/debian/dists/bookworm/main/binary-amd64", + "https://b.example.com/debian/dists/bookworm/main/binary-amd64", + } + if len(got) != 1 || !reflect.DeepEqual(got[0].Urls, want) { + t.Errorf("got %+v, want one repo with %q", got, want) + } +} + func TestGetPacmanRepos(t *testing.T) { // Skip if not on Arch system if _, err := os.Stat("/etc/pacman.conf"); os.IsNotExist(err) { diff --git a/python/patchman_client/client.py b/python/patchman_client/client.py index 52b8258..420bb8f 100755 --- a/python/patchman_client/client.py +++ b/python/patchman_client/client.py @@ -669,11 +669,12 @@ def _collect_apt_repos(self) -> None: pass # Build repo entries from resolved URLs - for priority, base_url, dist, component in http_entries: - if component.endswith('Packages'): + for priority, base_url, dist, apt_arch in http_entries: + base_url = base_url.rstrip('/') + if apt_arch.endswith('Packages'): url = f'{base_url}/{dist}' else: - url = f'{base_url}/dists/{dist}/binary-{self.arch}' + url = f'{base_url}/dists/{dist}/binary-{apt_arch}' repo_name = f'{os_name} {version_id} {self.arch} repo at {url}' diff --git a/tests/test_client_output.py b/tests/test_client_output.py index 2eafdeb..d91447c 100644 --- a/tests/test_client_output.py +++ b/tests/test_client_output.py @@ -1376,6 +1376,73 @@ def test_mirror_file_resolution(self, tmp_path=None): os.unlink(mirror_path) +class AptMirrorFileTrailingSlashTests(unittest.TestCase): + """Mirror file URLs with a trailing slash must not produce '//' in repo URLs, + and binary- must use the apt arch, not the machine arch.""" + + def setUp(self): + import tempfile + self.tmpdir = tempfile.mkdtemp() + self.addCleanup(__import__('shutil').rmtree, self.tmpdir) + mirror_file = os.path.join(self.tmpdir, 'mirrors.txt') + with open(mirror_file, 'w') as f: + f.write('http://deb.debian.org/debian/\n') + self.policy = ( + 'Package files:\n' + f' 500 mirror+file:{mirror_file} bookworm/main amd64 Packages\n' + ' release v=12,o=Debian,a=stable,n=bookworm,c=main,b=amd64\n' + ) + self.expected = 'http://deb.debian.org/debian/dists/bookworm/main/binary-amd64' + + def test_python_client(self): + import sys + sys.path.insert(0, os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), + 'python', + )) + from patchman_client.client import PatchmanClient + + client = PatchmanClient.__new__(PatchmanClient) + client.repos = [] + client.arch = 'x86_64' + client.os_string = 'Debian 12' + client.log = lambda *a, **kw: None + client._command_exists = lambda cmd: True + client._run_command = lambda cmd: (0, self.policy, '') + client._collect_apt_repos() + self.assertEqual(client.repos[0]['urls'], [self.expected]) + + def test_bash_client(self): + import subprocess + bindir = os.path.join(self.tmpdir, 'bin') + os.mkdir(bindir) + apt_cache = os.path.join(bindir, 'apt-cache') + with open(apt_cache, 'w') as f: + f.write(f"#!/bin/sh\ncat <<'EOF'\n{self.policy}EOF\n") + os.chmod(apt_cache, 0o755) + conf = os.path.join(self.tmpdir, 'patchman-client.conf') + with open(conf, 'w') as f: + f.write('server=https://pm.test\napi_key=test\n') + src = os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), + 'bash', 'patchman-client', + ) + # /run/patchman is not writable for unprivileged test runners + client = os.path.join(self.tmpdir, 'patchman-client') + with open(src) as f: + script = f.read() + self.assertIn('lock_dir=/run/patchman', script) + with open(client, 'w') as f: + f.write(script.replace('lock_dir=/run/patchman', f'lock_dir={self.tmpdir}/lock')) + env = dict(os.environ, PATH=f'{bindir}:{os.environ["PATH"]}') + result = subprocess.run( + ['bash', client, '-y', '-d', '-c', conf], + capture_output=True, text=True, timeout=60, env=env, + ) + self.assertIn(f"'500' '{self.expected}'", result.stdout) + self.assertNotIn('debian//dists', result.stdout) + + class BashDebFieldBranchingTests(unittest.TestCase): """Tests for deb vs rpm field format in bash build_repos_json."""