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

Jump to bottom

Replaces priority #1166

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Replaces priority #1166

wants to merge 4 commits into from

Conversation

xnox
Copy link
Contributor

@xnox xnox commented Apr 23, 2024 •
edited

This makes things awesome for python.

Melange Pull Request Template

Functional Changes

  • This change can build all of Wolfi without errors (describe results in notes)

Notes:

SCA Changes

  • Examining several representative APKs show no regression / the desired effect (details in notes)

Notes:

Linter

  • The new check is clean across Wolfi
  • The new check is opt-in or a warning

Notes:

@xnox xnox marked this pull request as draft April 23, 2024 23:56
@xnox
Add ReplacesPriroity like ProviderPriority, and allow substitutions
2f5a7d1 
This enables one to use variables, and range substitutions in the
ReplacesPriority and ProviderPriority.

Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@chainguard.dev>
@xnox xnox marked this pull request as ready for review April 26, 2024 23:05
xnox added a commit to xnox/os that referenced this pull request Apr 26, 2024
@xnox
Drop pybins and allow to install them all
27b22d6 
Requires chainguard-dev/melange#1166

Drop pybins, instead build all packages with commands in /usr/bin.
Set replaces, replaces-priority to ensure they are apk installable.
Also set provider-priority such that `apk add py3-pip` transparently
installs py3.12-pip alone.

This is all great, however not at all supported by apko. But imho
should be supported. As one simply has to sort packages in replaces
priority order prior to streaming. And allow overriding files between
packages that have replaces declared.

```console
...
(13/25) Installing python-3.10-base (3.10.14-r1)
(14/25) Installing py3.10-flit-core (3.9.0-r2)
(15/25) Installing py3.10-setuptools (69.5.1-r1)
(16/25) Installing py3.10-pip (24.0-r2)
(17/25) Installing python-3.11-base (3.11.9-r1)
(18/25) Installing py3.11-flit-core (3.9.0-r2)
(19/25) Installing py3.11-setuptools (69.5.1-r1)
(20/25) Installing py3.11-pip (24.0-r2)
(21/25) Installing python-3.12-base (3.12.3-r1)
(22/25) Installing py3.12-flit-core (3.9.0-r2)
(23/25) Installing py3.12-setuptools (69.5.1-r1)
(24/25) Installing py3.12-pip (24.0-r2)
(25/25) Installing py3-supported-pip (24.0-r2)
OK: 197 MiB in 40 packages

-rwxrwxr-x    1 root     root           946 Apr 26 22:22 /usr/bin/pip3.12
-rwxrwxr-x    1 root     root           946 Apr 26 22:22 /usr/bin/pip3.11
-rwxrwxr-x    1 root     root           946 Apr 26 22:22 /usr/bin/pip3.10
-rwxrwxr-x    1 root     root           940 Apr 26 22:22 /usr/bin/pip3
-rwxrwxr-x    1 root     root           938 Apr 26 22:22 /usr/bin/pip
```
smoser
smoser reviewed Apr 27, 2024
View reviewed changes
pkg/config/schema.json Outdated Show resolved Hide resolved
@xnox
schmea: validate priority integer strings, and update schema comment
31a4c30 
Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@chainguard.dev>
@xnox xnox requested a review from smoser April 29, 2024 11:59
@kaniini
Copy link
Contributor

kaniini commented May 15, 2024

with my apk-tools maintainer hat on, this looks fine

smoser
smoser requested changes May 16, 2024
View reviewed changes
Copy link
Contributor

@smoser smoser left a comment

Choose a reason for hiding this comment

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

Some doc on range would be nice. I can't seem to find anything in doc/ about range at all. But I understand you didn't add this.

It looks like it would be pretty easy to add test coverage to this in pkg/config/config_test.go . Could you add some coverage?

@xnox xnox added the blocked indicates there are blocking issues that need to be addressed before progress can be made label May 21, 2024
@xnox
Copy link
Contributor Author

xnox commented May 21, 2024

@kaniini thank you for your comments. "replaces_priority" is only available in the installed database as q, but it is not available in the APKINDEX, meaning looking at APKINDEX alone one cannot know how to order things ahead of time. Would that be just an oversight? as "provider_priority" is available under k in both installled database and apkindex.

Or is it optional and it wouldn't hurt anything to add it in the APKINDEX?

xnox
xnox commented May 23, 2024
View reviewed changes
Copy link
Contributor Author

@xnox xnox left a comment

Choose a reason for hiding this comment

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

we need this.

@xnox
Copy link
Contributor Author

xnox commented May 24, 2024

Some doc on range would be nice. I can't seem to find anything in doc/ about range at all. But I understand you didn't add this.

There is https://github.com/chainguard-dev/melange/blob/main/NEWS.md#major-changes-from-010-to-020 and also schema (but it just says that range is an acceptable key, without explaining which arguments it supports)

It looks like it would be pretty easy to add test coverage to this in pkg/config/config_test.go . Could you add some coverage?

Will check.

@xnox
tests: add range priority replacement tests
5e4d519 
Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@chainguard.dev>
@xnox xnox requested a review from smoser May 24, 2024 10:35
@xnox xnox removed the blocked indicates there are blocking issues that need to be addressed before progress can be made label May 24, 2024
@xnox
Copy link
Contributor Author

xnox commented May 24, 2024

Tests added. Docs are in academy and in the repo.

Will see where best to improve stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoser smoser Awaiting requested review from smoser

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@xnox @kaniini @smoser