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

Arch packages implementation #31037

Closed
wants to merge 46 commits into from

Conversation

ExplodingDragon
Copy link
Contributor

@ExplodingDragon ExplodingDragon commented May 21, 2024

close #25037

This Commit was created by d1nch8g (#25396).

This PR adds a package registry for Arch Linux packages with support for package files, signatures, and automatic pacman-database management.

Features:

  1. Push any tar.zst package and Gitea sign it.
  2. Delete endpoint for specific package version and all related files
  3. Supports trust levels with SigLevel = Required.
  4. Package UI with instructions to connect to the new pacman database and visualised package metadata

You can follow this tutorial to build a *.pkg.tar.zst package for testing

docs pr: gitea/docs#47

Co-authored-by: d1nch8g@ion.lc
Co-authored-by: @KN4CK3R
Co-authored-by: @silverwind
Co-authored-by: @mahlzahn

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 21, 2024
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 21, 2024
@ExplodingDragon ExplodingDragon marked this pull request as draft May 21, 2024 07:23
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/docs labels May 21, 2024
@github-actions github-actions bot added modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/migrations modifies/internal modifies/js modifies/dependencies labels May 21, 2024
@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 21, 2024
@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 21, 2024
@ExplodingDragon
Copy link
Contributor Author

ExplodingDragon commented May 21, 2024

TODO:

  • Package signatures should be managed by Gitea, custom signatures will result in inconsistencies with index signatures.
  • Signatures should not be saved under properties
  • Pacman DB should not be generated on-the-fly, it needs to be added when uploaded
  • We should not support overwrite uploads, which breaks the structure, and should perform a delete operation before uploading.
  • Fix documentation errors .

@ExplodingDragon ExplodingDragon marked this pull request as ready for review June 28, 2024 18:55
@ExplodingDragon
Copy link
Contributor Author

Thanks @wxiaoguang and @yp05327 , sorry.

@wxiaoguang
Copy link
Contributor

@KN4CK3R what's the next plan? Would you like to work on this PR, or propose a new one?

@lunny
Copy link
Member

lunny commented Nov 9, 2024

I think the main conflict is the router path, other codes could be refactored later. Maybe we can do a discussion about which change's routers are better?

@wxiaoguang

This comment was marked as outdated.

@wxiaoguang

This comment was marked as outdated.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 17, 2024

Let's start a fresh discussion @ExplodingDragon @KN4CK3R


I have made the tests in this PR pass KN4CK3R 's code: https://github.com/wxiaoguang/gitea/commits/feature-arch-KN4CK3R/ , see the last commit wxiaoguang@122001c (and I might still force push the last commit a few times to make it easier to read)

I marked the differences with // DIFF:, they are (this VS KN4CK3R's):

  1. DIFF:RepositoryKey: /repository.key VS /key
  2. DIFF:RepositoryName: this PR supports empty repo name and slashes in repo name. KN4CK3R's only supports a valid symbol name for repo name (it answers KN4CK3R's question: why the path handling is more complex here)
  3. DIFF:InvalidPackage: server response code 40x VS 500
  4. DIFF:PackageFileStore: sig file in package files VS not in
  5. DIFF:PackageFileNameExt: pkg VS pck
  6. DIFF:RepositoryDBFiles: [..../desc] VS [.../files, ..../desc]
  7. DIFF:DeletePackageURL: DELETE /test/1.0.0-1/x86_64 VS /x86_64/test-1.0.0-1-x86_64.pck.tar.zst
  8. DIFF:OtherPackageType: this PR supports xz and gz, KN4CK3R's doesn't answered above: archlinuxarm uses xz.

So what implementation should we take? Some differences are quite important (eg: RepositoryName, DeletePackageURL)

@wxiaoguang
Copy link
Contributor

Overall I think this PR is good enough, if the differences in #31037 (comment) don't block.

@wxiaoguang
Copy link
Contributor

@KN4CK3R last call if the differences in #31037 (comment) don't matter.

@KN4CK3R
Copy link
Member

KN4CK3R commented Nov 21, 2024

I will push some changes later.

Edit: Changing the existing was harder than expected. Therefore I added the slashed reponame and different compression types to my PR. These changes could be integrated easily but are not pushed yet because I need to extend the tests.

@KN4CK3R
Copy link
Member

KN4CK3R commented Nov 27, 2024

I pushed the changes (coauthored by @ExplodingDragon) some days ago and removed the docs file. Maybe someone would like to have a look:
main...KN4CK3R:gitea:feature-arch

@wxiaoguang
Copy link
Contributor

Thank you, are there some details about the DIFF #31037 (comment) ?

DIFF:RepositoryName: this PR supports empty repo name and slashes in repo name. KN4CK3R's only supports a valid symbol name for repo name (it answers KN4CK3R's question: why the path handling is more complex here)

ExplodingDragon's support empty repo name, the new one doesn't support? (To be clear, whether it should be supported?)

DIFF:DeletePackageURL: DELETE /test/1.0.0-1/x86_64 VS /x86_64/test-1.0.0-1-x86_64.pck.tar.zst

Which URL is correct (or better) to delete?


And one more thing, in your routers/api/packages/api.go, you bloated the api.go again ... I have managed to move these complex logic into handlers in 84ff353 , I do not think it is right to write a lot of code in api.go

@KN4CK3R
Copy link
Member

KN4CK3R commented Nov 28, 2024

ExplodingDragon's support empty repo name, the new one doesn't support? (To be clear, whether it should be supported?)

I removed the empty check now. Empty repos are fine too.

Which URL is correct (or better) to delete?

There is no correct because there is no official spec. Deleting a package should rarely happen. It may be easier for the user to just have the package name and version in the url instead of crafting the full filename. Providing the filename makes the code easier. Removing this endpoint may be even easier.

you bloated the api.go again

The only reason for this code is because chi doesn't support /*/{param}. Therefore it's chi related and belongs into the api.go.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 28, 2024

Which URL is correct (or better) to delete?

There is no correct because there is no official spec. Deleting a package should rarely happen. It may be easier for the user to just have the package name and version in the url instead of crafting the full filename. Providing the filename makes the code easier. Removing this endpoint may be even easier.

DELETE /test/1.0.0-1/x86_64 looks clearer to me.

The question is: does the full&extension name matter? If the full&extension name matters, then it must use DELETE /x86_64/test-1.0.0-1-x86_64.pkg.tar.zst (but, why x86_64 duplicates?). Otherwise, DELETE /test/1.0.0-1/x86_64 seems more general and works for all extensions.

If we'd like to make it "providing the filename makes the code easier., removing this endpoint may be even easier.", maybe it should be something like DELETE /test-1.0.0-1-x86_64.pkg.tar.zst without the "arch"? (I don't quite understand the code, correct me if I was wrong)


you bloated the api.go again

The only reason for this code is because chi doesn't support /*/{param}. Therefore it's chi related and belongs into the api.go.

I do not quite agree, we can't blame chi everytime and keep adding more code to the router init function, it definitely will make the api.go become a monster and more and more difficult to maintain.

(That's just my opinion and I won't block it.)

@wxiaoguang
Copy link
Contributor

And one more question, how to continue? Would you like to push your changes here, or create a new PR?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 2, 2024

@KN4CK3R maybe 1.23 is going to be frozen in a week. The last 2 questions:

  • Which one should be used to "DELETE"?
    • DELETE /.../{repo-group}/test/1.0.0-1/x86_64: simple and no extension name (ExplodingDragon's)
    • DELETE /.../{repo-group}/x86_64/test-1.0.0-1-x86_64.pkg.tar.zst: have full file name and duplicate architecture (KN4CK3R's)
    • DELETE /.../{repo-group}/test-1.0.0-1-x86_64.pkg.tar.zst: only full file name, no duplicate architecture?
  • How to continue?
    • push KN4CK3R's changes here?
    • create a new PR from KN4CK3R's branch?

@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 2, 2024

I already updated the delete endpoint in my code. If @ExplodingDragon is fine with it, I would like to create a PR.

@wxiaoguang
Copy link
Contributor

I already updated the delete endpoint in my code. If @ExplodingDragon is fine with it, I would like to create a PR.

According to #31037 (comment)

I'm ok with both, 3 is better for me, I don't have much time to work on things, I'll close this pr later, other things are up to you to determine.

🙏

@ExplodingDragon
Copy link
Contributor Author

ExplodingDragon commented Dec 3, 2024

This pr has been implemented by @KN4CK3R (#32692).

@GiteaBot GiteaBot removed this from the 1.23.0 milestone Dec 3, 2024
lunny pushed a commit that referenced this pull request Dec 4, 2024
Close #25037
Close #31037

This PR adds a Arch package registry usable with pacman.

![grafik](https://github.com/user-attachments/assets/81cdb0c2-02f9-4733-bee2-e48af6b45224)

Rewrite of #25396 and #31037. You can follow [this
tutorial](https://wiki.archlinux.org/title/Creating_packages) to build a
package for testing.

Docs PR: https://gitea.com/gitea/docs/pulls/111

Co-authored-by: [d1nch8g@ion.lc](mailto:d1nch8g@ion.lc)
Co-authored-by: @ExplodingDragon

---------

Co-authored-by: dancheg97 <dancheg97@fmnx.su>
Co-authored-by: dragon <ExplodingFKL@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@ExplodingDragon ExplodingDragon deleted the pacman-packages branch December 9, 2024 03:44
@wxiaoguang
Copy link
Contributor

And one more thing, in your routers/api/packages/api.go, you bloated the api.go again ... I have managed to move these complex logic into handlers in 84ff353 , I do not think it is right to write a lot of code in api.go

The only reason for this code is because chi doesn't support /*/{param}. Therefore it's chi related and belongs into the api.go.

To make the routes clearer: Refactor arch route handlers #32972 (the first step) , I am sure we could have a quite clear router init function soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arch linux packages
8 participants