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

alpine: add capabilities to download packages via HTTP #80

Merged
merged 1 commit into from Aug 30, 2020

Conversation

Cogitri
Copy link
Contributor

@Cogitri Cogitri commented May 15, 2020

I hope I don't have too many style errors this time, the style is still a bit foreign to me :)

@ximion
Copy link
Owner

ximion commented May 15, 2020

Looks sane to me - have you tested this? I don't know that much about Alpine, so in the long run it may be useful to give you commit access to the repo in case the backend needs regular maintenance.
Also, I wonder why the CI doesn't seem to report the build status of PRs anymore...

@Cogitri
Copy link
Contributor Author

Cogitri commented May 15, 2020

Looks sane to me - have you tested this?

Ah, was just in the progress of writing a comment :)

I've tested this locally on a smaller repo. I'm currently waiting for infra's answer what official mirror I can test this against without DOSing anything by accident ^^

I don't know that much about Alpine, so in the long run it may be useful to give you commit access to the repo in case the backend needs regular maintenance

Oh, that'd be appreciated. There's currently work going on for the next major version of our package manager that'll probably change a few things in the repo index format, but I think other than that there won't be too many adjustments required

@ximion
Copy link
Owner

ximion commented May 15, 2020

If you have a cache filled with data, you can run asgen forget one one package, and then run the process command again. Just the one package should be worked on in that case.

I like that you made use of ranges in this patch - when I was writing asgen initially (porting away from Python to increase speed) I didn't know all that much about D, so many areas are C/old-C++-like and could be rewritten in more idiomatic D (but there's not much of a point in doing that, for now)

@Cogitri
Copy link
Contributor Author

Cogitri commented May 15, 2020

So I just ran appstream-generator against our repos (without cache to see if it works for everything) and that worked fine after the minor fix I just pushed (forgot to put suite in the URL for the package), so I guess this should be good to go.

Glad you like the approach, I'm currently reading through the D cookbook, so it was nice that I was able to apply what I read a few days ago already :)

@z3ntu
Copy link

z3ntu commented Aug 29, 2020

Any update on this PR? The functionality seems to work fine

@z3ntu
Copy link

z3ntu commented Aug 29, 2020

Actually there seems to be a bug here that when the maintainer field of a package isn't set, the previous(?) maintainer field gets used for that package. I believe this patch could fix it:

diff --git a/src/asgen/backends/alpinelinux/apkindexutils.d b/src/asgen/backends/alpinelinux/apkindexutils.d
index 37b202e..b273e03 100644
--- a/src/asgen/backends/alpinelinux/apkindexutils.d
+++ b/src/asgen/backends/alpinelinux/apkindexutils.d
@@ -66,6 +66,7 @@ private:
         foreach (currentLine; this.lines[this.lineDelta .. $]) {
             iterations++;
             if (currentLine == "") {
+                this.currentBlock = ApkIndexBlock();
                 // next block for next package started
                 break;
             } if (currentLine.canFind (":")) {

I can't test at the moment on Arch Linux because when I launch appstream-generator it crashes fish: “./builddir/src/asgen/appstream-…” terminated by signal SIGILL (Illegal instruction) (0x00007ffff65d6ed0 in std::type_info::__is_pointer_p() const () from /usr/lib/libdruntime-ldc-shared.so.92) - I believe this is a toolchain issue or something and not related with this PR.

@ximion
Copy link
Owner

ximion commented Aug 29, 2020

Oh no, it looks like this PR fell through the cracks - I'm very sorry, thanks for the ping!
I think meanwhile the downloader code has been refactored, but if this code still works I'll merge it. Aside from the patch mentioned above I can't see any reason to not merge it.

That SIGILL thing is definitely a toolchain issue - maybe your compiler builds for a CPU architecture that you aren't running (i486 vs i686 etc.)? It's in the D runtime, so I guess almost any D software will fail to run like this.

ximion
ximion approved these changes Aug 30, 2020
@ximion
Copy link
Owner

ximion commented Aug 30, 2020

LGTM, sorry for taking so long to merge this one!

@ximion ximion merged commit 7f29630 into ximion:master Aug 30, 2020
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.

None yet

3 participants