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

Propagate install_if and provider_priority to APKINDEX #28899

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Propagate install_if and provider_priority to APKINDEX #28899

merged 3 commits into from
Feb 5, 2024

Conversation

bugaevc
Copy link
Contributor

@bugaevc bugaevc commented Jan 22, 2024

Resolves #28704

Example of an entry in the generated APKINDEX file:

C:Q1xCO3H9LTTEbhKt9G1alSC87I56c=
P:hello
V:2.12-r1
A:x86_64
T:The GNU Hello program produces a familiar, friendly greeting
U:https://www.gnu.org/software/hello/
L:GPL-3.0-or-later
S:15403
I:36864
o:hello
m:
t:1705934118
D:so:libc.musl-x86_64.so.1
p:cmd:hello=2.12-r1
i:foobar=1.0 !baz
k:42

the i: and k: entries are new.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 22, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 22, 2024
@bugaevc
Copy link
Contributor Author

bugaevc commented Jan 22, 2024

cc @joniw @mintyhippoxyz

@KN4CK3R KN4CK3R added type/enhancement An improvement of existing functionality topic/packages labels Jan 23, 2024
KN4CK3R
KN4CK3R requested changes Jan 23, 2024
View reviewed changes
Copy link
Member

@KN4CK3R KN4CK3R left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. This requires a migration because now the stored metadata is incompatible with the new struct.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 23, 2024
@lunny lunny added type/bug and removed type/bug labels Jan 24, 2024
@bugaevc
Copy link
Contributor Author

bugaevc commented Jan 27, 2024

Looks like install_if should be a single string and not an array after all: even though it has the same general format as depends/provides, abuild stores those as arrays (one per line), but formats install_if into a single space-separated line. So it makes sense to do just the same in the model. This also sidesteps the need for a migration.

PTAL

delvh
delvh reviewed Jan 31, 2024
View reviewed changes
modules/packages/alpine/metadata.go Outdated
case "replaces_priority":
n, err := strconv.ParseInt(value, 10, 64)
if err == nil {
p.FileMetadata.ReplacesPriority = n
Copy link
Member

Choose a reason for hiding this comment

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

Did I miss something, or is the ReplacesPriority currently unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is unused indeed. It is a part of Alpine package metadata, so it makes sense to parse and store it alongside all the other package metadata, but there isn't anything else being done with it currently. Specifically, it looks like there's no field for replaces_priority in the APKINDEX file, so ReplacesPriority is not used when generating APKINDEX.

I guess ReplacesPriority could show up in some HTTP API request if there is one that returns package file metadata, or if one is added in the future.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I recommend adding a comment to the field stating exactly that.

Copy link
Member

Choose a reason for hiding this comment

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

I removed the field as it's not used in the index spec at the moment. We can add it if it is used in future.

https://wiki.alpinelinux.org/wiki/Apk_spec#APKINDEX_Format

KN4CK3R
KN4CK3R reviewed Feb 3, 2024
View reviewed changes
modules/packages/alpine/metadata.go Outdated Show resolved Hide resolved
modules/packages/alpine/metadata.go Outdated Show resolved Hide resolved
KN4CK3R
KN4CK3R approved these changes Feb 3, 2024
View reviewed changes
Copy link
Member

@KN4CK3R KN4CK3R left a comment

Choose a reason for hiding this comment

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

The migration is not needed, the field is just 0.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Feb 3, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 4, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 5, 2024
@lunny lunny enabled auto-merge (squash) February 5, 2024 05:28
@lunny lunny merged commit 2da233a into go-gitea:main Feb 5, 2024
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Feb 5, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 5, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 7, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
f3b61f2 
* giteaofficial/main:
  Hide code links on release page if user cannot read code (go-gitea#29064)
  [skip ci] Updated translations via Crowdin
  Don't do a full page load when clicking `Watch` or `Star` (go-gitea#29001)
  Remove useless template file (go-gitea#29053)
  Fix typos in the documentation (go-gitea#29048)
  Move some repository transfer functions to service layer (go-gitea#28855)
  Propagate install_if and provider_priority to APKINDEX (go-gitea#28899)
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@bugaevc @KN4CK3R @silverwind
Propagate install_if and provider_priority to APKINDEX (go-gitea#28899)
caeca20 
Resolves go-gitea#28704

Example of an entry in the generated `APKINDEX` file:

```
C:Q1xCO3H9LTTEbhKt9G1alSC87I56c=
P:hello
V:2.12-r1
A:x86_64
T:The GNU Hello program produces a familiar, friendly greeting
U:https://www.gnu.org/software/hello/
L:GPL-3.0-or-later
S:15403
I:36864
o:hello
m:
t:1705934118
D:so:libc.musl-x86_64.so.1
p:cmd:hello=2.12-r1
i:foobar=1.0 !baz
k:42
```

the `i:` and `k:` entries are new.

---------

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh left review comments

@KN4CK3R KN4CK3R KN4CK3R approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/packages type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

Alpine package registry: Not all possible fields are present in generated APKINDEX.tar.gz
6 participants
@bugaevc @KN4CK3R @wxiaoguang @delvh @lunny @GiteaBot