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.

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 bug in dependency resolution #1311

Merged
merged 3 commits into from
Jul 11, 2024

Conversation

5hir0kur0
Copy link
Contributor

@5hir0kur0 5hir0kur0 commented Jul 10, 2024

Fixes #1312.

Aptly has a bug in its dependency resolution where certain dependencies do not get added even if they are not satisfied.

Steps to reproduce (tested with latest master):

git clone ...
go build -o aptly.dev main.go
./aptly.dev mirror create -ignore-signatures -filter='python3-pep517|python3|black' -architectures=amd64 -filter-with-deps test1 https://snapshot.debian.org/archive/debian/20240707T211122Z/ bookworm
./aptly.dev mirror update -ignore-signatures test1
./aptly.dev -with-packages mirror show test1 | grep python3-tomli

The package python-pep517 depends on python3-tomli:

Package: python3-pep517
Source: pep517
Version: 0.13.0-2
Installed-Size: 82
Maintainer: Debian Python Team <[email protected]>
Architecture: all
Depends: python3-importlib-metadata, python3-tomli, python3-zipp, python3:any
[...]

But Aptly does not include python3-tomli in the mirror.

This is because black includes the following dependency: python3-tomli | python3 (>> 3.11).
This dependency is satisfied by including python3, but as a side effect of evaluating this dependency, Aptly includes python3-tomli in the cache in VerifyDependencies.
The bug is that this causes python3-tomli to be excluded, even though its cache entry is false (i.e., not satisfied).

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

This was causing issues for us because Aptly did not download all dependencies even with -filter-with-deps.

I fixed this bug by just checking for the satisfied status instead of also requiring that the dependency not be included in the cache.

diff --git a/deb/list.go b/deb/list.go
index ec14bb98..89fc7d08 100644
--- a/deb/list.go
+++ b/deb/list.go
@@ -351,7 +351,7 @@ func (l *PackageList) VerifyDependencies(options int, architectures []string, so
                                                cache[hash] = satisfied
                                        }

-                                   if !satisfied && !ok {
+                                 if !satisfied {
                                                variantsMissing = append(variantsMissing, dep)
                                        }

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

@neolynx
Copy link
Member

neolynx commented Jul 10, 2024

thanks a lot for catching and fixing this !

looks like the tests were relying on duplicated packages, hence they fail now...

@neolynx neolynx force-pushed the feature/fix-dependency-resolution branch from a4f9133 to 262567a Compare July 10, 2024 16:32
@neolynx
Copy link
Member

neolynx commented Jul 10, 2024

I rebased and updated the tests.

Could you maybe check whether these changes are OK ? the original code seems to have missed dependencies on bind, emacs22, kde, ttf-thryomanes, xpdf-reader. Since some of them are meta packages, is this really the desired dependency filtering ?

@neolynx neolynx requested review from neolynx and a team July 10, 2024 21:39
@5hir0kur0
Copy link
Contributor Author

@neolynx

Thank you! Looks good to me.

As for the question about virtual packages, I am not sure, maybe it depends on how the callers of VerifyDependencies handle virtual packages?

But in general I think that it has been possible for virtual/meta packages to be returned from VerifyDependencies since before my change, so this should not be a new phenomenon.

@neolynx neolynx merged commit 8029305 into aptly-dev:master Jul 11, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in dependency resolution with -filter-with-deps
2 participants