Skip to content
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

Fix package name handling to retain version and strip ‘@’ suffix #1472

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Fix package name handling to retain version and strip ‘@’ suffix #1472

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 33 additions & 4 deletions pkg/build/lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ type resolved struct {
arch string
packages sets.Set[string]
versions map[string]string
pinned map[string]string
provided map[string]sets.Set[string]
}

Expand All @@ -124,21 +125,37 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
originalPackages := resolved{
packages: make(sets.Set[string], len(originals)),
versions: make(map[string]string, len(originals)),
pinned: make(map[string]string, len(originals)),
}

byArch := map[string][]string{}

for _, orig := range originals {
name := orig
version := ""
pinned := ""

// The function we want from go-apk is private, but these are all the
// special characters that delimit the package name from the cosntraint
// so lop off the package name and stick the rest of the constraint into
// the versions map.
if idx := strings.IndexAny(orig, "=<>~"); idx >= 0 {
name = orig[:idx]
version = orig[idx:]
}

// Extract pinned version if present
if idx := strings.IndexAny(orig, "@"); idx >= 0 {
pinned = orig[idx:]
}

// Remove pinned suffix from name and version
name = strings.TrimSuffix(name, pinned)
version = strings.TrimSuffix(version, pinned)

originalPackages.packages.Insert(name)
originalPackages.versions[name] = strings.TrimPrefix(orig, name)
originalPackages.versions[name] = version
originalPackages.pinned[name] = pinned
}

// Start accumulating using the first entry, and unify it with the other
Expand Down Expand Up @@ -226,7 +243,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
// package constraint including the operator.
for _, pkg := range sets.List(missing) {
if ver := originalPackages.versions[pkg]; ver != "" {
pl = append(pl, fmt.Sprintf("%s%s", pkg, ver))
if pin := originalPackages.versions[pkg]; pin != "" {
pl = append(pl, fmt.Sprintf("%s%s%s", pkg, ver, pin))
Copy link

Choose a reason for hiding this comment

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

This produces an invalid world when using tagged repositories like @local or @edge.

The order needs to be

- pl = append(pl, fmt.Sprintf("%s%s%s", pkg, ver, pin))
+ pl = append(pl, fmt.Sprintf("%s%s%s", pkg, pin, ver))

as per apk-world(5):

This is a plaintext file with one constraint using dependency notation per line. Each line has the format: name{@tag}{[<>~=]version}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @emilylange

When I tested it with @local package, the format you shared didn't work. This is the reason I kept that like that. However, changing the order doesn't break the local package build since it is not the case for local built packages.

With @local package repository below example config does not work.

contents:
  repositories:
    - '@local ./packages'
  keyring:
    - melange.rsa.pub
  packages:
    - world@local=7.0.0-r0

This is the error message. This error happens before the changes.

2025/01/24 19:25:15 DEBU got 1 indexes:
./packages/x86_64/APKINDEX.tar.gz
Error: locking config: resolving apk packages: for arch "amd64": solving "world@local=7.0.0-r0" constraint: nothing provides "world@local=7.0.0-r0"
2025/01/24 19:25:15 INFO error during command execution: locking config: resolving apk packages: for arch "amd64": solving "world@local=7.0.0-r0" constraint: nothing provides "world@local=7.0.0-r0"

However, this config works but it doesn't match with the format.

contents:
  repositories:
    - '@local ./packages'
  keyring:
    - melange.rsa.pub
  packages:
    - world=7.0.0-r0@local

Can you share the failing config?

Copy link

Choose a reason for hiding this comment

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

I was referring to the resulting /etc/apk/world.

But you are right that this particular line I commented on isn't the culprit, as after looking a lot more into this, I can confidently say that this goes way deeper.

It is inherently confusing to have slight ordering differences across the codebase.

A minimal example and reproducer is:

contents:
  repositories:
    - https://dl-cdn.alpinelinux.org/alpine/edge/main
    - '@testing https://dl-cdn.alpinelinux.org/alpine/edge/testing'
  packages:
    - alpine-base@testing

entrypoint:
  command: /bin/ash -l

archs:
  - amd64

Thanks to this PR here, apko lock and apko build are no longer failing. That's lovely and all.
But it doesn't fix the underlying cause, unfortunately.

The resulting /etc/apk/world look something like this (v0.23.0, based on the APKINDEX at the time of writing):

aef8b91a311a:/# cat /etc/apk/world 
alpine-base=3.22.0_alpha20250108-r0@testing
alpine-baselayout-data=3.6.8-r1
alpine-baselayout=3.6.8-r1
alpine-conf=3.19.2-r0
alpine-keys=2.5-r0
alpine-release=3.22.0_alpha20250108-r0
apk-tools=2.14.9-r0
busybox-binsh=1.37.0-r13
busybox-ifupdown=1.37.0-r13
busybox-mdev-openrc=1.37.0-r13
busybox-openrc=1.37.0-r13
busybox-suid=1.37.0-r13
busybox=1.37.0-r13
ca-certificates-bundle=20241121-r1
libcap2=2.73-r0
libcrypto3=3.3.2-r4
libssl3=3.3.2-r4
mdev-conf=4.7-r0
musl-utils=1.2.5-r9
musl=1.2.5-r9
openrc=0.56-r0
scanelf=1.3.8-r1
ssl_client=1.37.0-r13
zlib=1.3.1-r2

As pointed out in my previous comment, alpine-base=3.22.0_alpha20250108-r0@testing is an invalid entry as per apk-world(5).

If you run apk upgrade, apk fix or apk add <some other package> in a given container with that /etc/apk/world, apk will remove that entry and dependencies.

aef8b91a311a:/# apk upgrade
fetch https://dl-cdn.alpinelinux.org/alpine/edge/testing/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/edge/main/x86_64/APKINDEX.tar.gz
(1/24) Purging alpine-base (3.22.0_alpha20250108-r0)
(2/24) Purging alpine-baselayout (3.6.8-r1)
(3/24) Purging alpine-baselayout-data (3.6.8-r1)
(4/24) Purging alpine-release (3.22.0_alpha20250108-r0)
(5/24) Purging alpine-keys (2.5-r0)
(6/24) Purging apk-tools (2.14.9-r0)
(7/24) Purging ca-certificates-bundle (20241121-r1)
(8/24) Purging busybox-mdev-openrc (1.37.0-r13)
(9/24) Purging mdev-conf (4.7-r0)
(10/24) Purging busybox-openrc (1.37.0-r13)
(11/24) Purging busybox-suid (1.37.0-r13)
(12/24) Purging musl-utils (1.2.5-r9)
(13/24) Purging scanelf (1.3.8-r1)
(14/24) Purging alpine-conf (3.19.2-r0)
(15/24) Purging openrc (0.56-r0)
(16/24) Purging busybox-binsh (1.37.0-r13)
(17/24) Purging busybox (1.37.0-r13)
(18/24) Purging libcap2 (2.73-r0)
(19/24) Purging busybox-ifupdown (1.37.0-r13)
(20/24) Purging zlib (1.3.1-r2)
(21/24) Purging ssl_client (1.37.0-r13)
(22/24) Purging libssl3 (3.3.2-r4)
(23/24) Purging libcrypto3 (3.3.2-r4)
(24/24) Purging musl (1.2.5-r9)
OK: 17592186044408 MiB in 0 packages
aef8b91a311a:/# cat /etc/apk/world 
/bin/ash: cat: not found
aef8b91a311a:/# read file < /etc/apk/world ; echo $file

aef8b91a311a:/#

With alpine-base being the only package in the example provided, apk will delete everything, resulting in an unusable environment.

So far, I came up with the following diff to fix the underlying cause.
Though I would have to check

diff --git a/pkg/apk/apk/version.go b/pkg/apk/apk/version.go
index 185dbf7..5d88922 100644
--- a/pkg/apk/apk/version.go
+++ b/pkg/apk/apk/version.go
@@ -37,7 +37,7 @@ import (
 
 var (
        versionRegex     = regexp.MustCompile(`^([0-9]+)((\.[0-9]+)*)([a-z]?)((_alpha|_beta|_pre|_rc)([0-9]*))?((_cvs|_svn|_git|_hg|_p)([0-9]*))?((-r)([0-9]+))?$`)
-       packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(([=><~]+)([^@]+))?(@([a-zA-Z0-9]+))?$`)
+       packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(@([a-zA-Z0-9]+))?(([=><~]+)([^@]+))?$`)
 )
 
 func init() {
@@ -367,12 +367,12 @@ func resolvePackageNameVersionPin(pkgName string) parsedConstraint {
        // layout: [full match, name, =version, =|>|<, version, @pin, pin]
        p := parsedConstraint{
                name:    parts[0][1],
-               version: parts[0][4],
-               pin:     parts[0][6],
+               pin:     parts[0][3],
+               version: parts[0][6],
                dep:     versionAny,
        }
 
-       matcher := parts[0][3]
+       matcher := parts[0][5]
        if matcher != "" {
                // we have an equal
                switch matcher {
diff --git a/pkg/apk/apk/version_test.go b/pkg/apk/apk/version_test.go
index 1e99665..9e6afd4 100644
--- a/pkg/apk/apk/version_test.go
+++ b/pkg/apk/apk/version_test.go
@@ -933,8 +933,8 @@ func TestResolverPackageNameVersionPin(t *testing.T) {
                {"name<1.2.3", "name", "1.2.3", versionLess, ""},
                {"name>=1.2.3", "name", "1.2.3", versionGreaterEqual, ""},
                {"name<=1.2.3", "name", "1.2.3", versionLessEqual, ""},
-               {"name@edge=1.2.3", "name@edge=1.2.3", "", versionAny, ""}, // wrong order, so just returns the whole thing
-               {"name=1.2.3@community", "name", "1.2.3", versionEqual, "community"},
+               {"name@edge=1.2.3", "name", "1.2.3", versionEqual, "edge"},
+               {"name=1.2.3@community", "name=1.2.3@community", "", versionAny, ""}, // wrong order, so just returns the whole thing
        }
 
        for _, tt := range tests {
diff --git a/pkg/build/lock.go b/pkg/build/lock.go
index e00e858..c36e096 100644
--- a/pkg/build/lock.go
+++ b/pkg/build/lock.go
@@ -145,8 +145,8 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
                }
 
                // Extract pinned version if present
-               if idx := strings.IndexAny(orig, "@"); idx >= 0 {
-                       pinned = orig[idx:]
+               if idx := strings.IndexAny(name, "@"); idx >= 0 {
+                       pinned = name[idx:]
                }
 
                // Remove pinned suffix from name and version
@@ -256,10 +256,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
        // Append all of the resolved and unified packages with an exact match
        // based on the resolved version we found.
        for _, pkg := range sets.List(acc.packages) {
-               pkgName := fmt.Sprintf("%s=%s", pkg, acc.versions[pkg])
+               pkgName := pkg
                if pin := originalPackages.pinned[pkg]; pin != "" {
                        pkgName = fmt.Sprintf("%s%s", pkgName, pin)
                }
+               pkgName = fmt.Sprintf("%s=%s", pkgName, acc.versions[pkg])
                pl = append(pl, pkgName)
        }
        // Sort the package list explicitly with the `=` included.
@@ -274,10 +275,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
        for _, input := range inputs {
                pl := make([]string, 0, len(input.packages))
                for _, pkg := range sets.List(input.packages) {
-                       pkgName := fmt.Sprintf("%s=%s", pkg, input.versions[pkg])
+                       pkgName := pkg
                        if pin := originalPackages.pinned[pkg]; pin != "" {
                                pkgName = fmt.Sprintf("%s%s", pkgName, pin)
                        }
+                       pkgName = fmt.Sprintf("%s=%s", pkgName, input.versions[pkg])
                        pl = append(pl, pkgName)
                }
                // Sort the package list explicitly with the `=` included.
@@ -304,4 +306,4 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
 }
 
 // Copied from go-apk's version.go
-var packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(([=><~]+)([^@]+))?(@([a-zA-Z0-9]+))?$`)
+var packageNameRegex = regexp.MustCompile(`^([^@=><~]+)(@([a-zA-Z0-9]+))?(([=><~]+)([^@]+))?$`)

Though given the lack of code coverage, this could also cause regressions in some unexpected way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emilylange Thanks for the detailed explanation! I think this needs its own ticket and maybe even a discussion in the #apko Slack channel. Since this is a merged ticket, there’s a good chance the right people might not see it. Would be great to get more visibility on this.

} else {
pl = append(pl, fmt.Sprintf("%s%s", pkg, ver))
}
} else {
pl = append(pl, pkg)
}
Expand All @@ -235,7 +256,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
// Append all of the resolved and unified packages with an exact match
// based on the resolved version we found.
for _, pkg := range sets.List(acc.packages) {
pl = append(pl, fmt.Sprintf("%s=%s", pkg, acc.versions[pkg]))
pkgName := fmt.Sprintf("%s=%s", pkg, acc.versions[pkg])
if pin := originalPackages.pinned[pkg]; pin != "" {
pkgName = fmt.Sprintf("%s%s", pkgName, pin)
}
pl = append(pl, pkgName)
}
// Sort the package list explicitly with the `=` included.
// This is because (foo, foo-bar) sorts differently than (foo=1, foo-bar=1)
Expand All @@ -249,7 +274,11 @@ func unify(originals []string, inputs []resolved) (map[string][]string, map[stri
for _, input := range inputs {
pl := make([]string, 0, len(input.packages))
for _, pkg := range sets.List(input.packages) {
pl = append(pl, fmt.Sprintf("%s=%s", pkg, input.versions[pkg]))
pkgName := fmt.Sprintf("%s=%s", pkg, input.versions[pkg])
if pin := originalPackages.pinned[pkg]; pin != "" {
pkgName = fmt.Sprintf("%s%s", pkgName, pin)
}
pl = append(pl, pkgName)
}
// Sort the package list explicitly with the `=` included.
// This is because (foo, foo-bar) sorts differently than (foo=1, foo-bar=1)
Expand Down
Loading