media_baseurl is always prepended to screenshots urls #395
Comments
|
I have a dumb fix that works, but I'm not sure this is absolutely correct way to handle this situation diff --git a/src/as-image.c b/src/as-image.c
index 012d243b..ee9fe9db 100644
--- a/src/as-image.c
+++ b/src/as-image.c
@@ -266,6 +266,11 @@ as_image_set_locale (AsImage *image, const gchar *locale)
as_ref_string_assign_safe (&priv->locale, locale);
}
+static gboolean is_url_absolute (const gchar *url)
+{
+ return g_str_has_prefix (url, "https://") || g_str_has_prefix (url, "http://");
+}
+
/**
* as_image_load_from_xml:
* @image: a #AsImage instance.
@@ -333,8 +338,8 @@ as_image_load_from_xml (AsImage *image, AsContext *ctx, xmlNode *node, GError **
}
g_strstrip (content);
- if (!as_context_has_media_baseurl (ctx)) {
- /* no baseurl, we can just set the value as URL */
+ if (!as_context_has_media_baseurl (ctx) || is_url_absolute (content)) {
+ /* no baseurl or URL is already absolute path, we can just set the value as URL */
as_image_set_url (image, content);
} else {
/* handle the media baseurl */After applying this I see correct output from my test program P.S need to not forget to remove |
|
The AppStream behavior is actually correct: If there is a media baseurl, it must be prepended to every media URL segment in a metadata file (great performance improvement when processing thousands of components). |
|
The data is taken without modifications from what was generated by appstream-generator (with alpinelinux backend). |
|
Yeah, that's wrong, there is a bug somewhere. What is your settings file for asgen that you used to generate this data? |
|
It's generated on Alpine infra, using this I believe https://gitlab.alpinelinux.org/alpine/infra/compose/appstream-generator/-/blob/master/asgen-config.json |
|
I had a look at this, but still don't know how this could happen - the code shouldn't be able to take a branch where the media_baseurl is set and source image URLs from screenshots are taken without using the media_baseurl and the media_baseurl is correctly used for any generated thumbnails. |
|
Thank you for looking into this! I'll try to run generator locally, but I haven't tried it yet myself and I'm afraid it will take some time. P.S. ktorrent is not the only package with this issue, many org.kde.* packages I looked at had similar screenshot URLs |
Yeah, I bet! There's a bug here somewhere, and since I really don't think it has anything to do with Alpine, I should be able to reproduce it. I may have a bit more time the next few days to try this properly. |
I can confirm that this is happening with appstream-generator=0.8.8 and appstream=0.15.3. I "forgot" d-feet package in main, regenerated, and the issue is still there: https://appstream.pcorreag.tk/data/edge/main/Components-x86_64.xml.gz |
|
Would you like us to try with the latest master commit? |
You can, but I don't think this will do anything - I just fixed another potential issue that nobody encountered yet that I found while reviewing the code. |
Absolutely! I modified the config Alexei pointed to. I kept only "main" section and "aarch64" and "x86_64" architectures to reduce cruft from the output. |
|
No matter what I try, I can't reproduce this issue at all... {
"ProjectName": "AlpineLinux",
"ArchiveRoot": "http://dl-cdn.alpinelinux.org/alpine/",
"MediaBaseUrl": "https://appstream.alpinelinux.org/media",
"Backend": "alpinelinux",
"Suites":
{
"edge":
{
"sections": ["main"],
"architectures": ["x86_64"]
}
},
"Icons":
{
"48x48": {"cached": false, "remote": true},
"64x64": {"cached": true, "remote": true},
"128x128": {"cached": false, "remote": true}
}
}Using |
|
You can try to take ximion/appstream-generator@cc4df09 (fixes a few other issues), but I don't think this will do anything... |
|
@ximion, I have tried to reproduce your problems... And it is indeed a problem with alpine. Either muslc, some part of the D stack or something in between is messing up. The same configuration with 0.8.8+0.15.3 from debian sid and from alpine edge yield different results, running both from scratch and the same configuration file. The run on debian sid outputs the component file you expect, while the one in alpine edge outputs what we are reporting. I guess unless you feel like spending more time on this, we will have to report to alpine and see how can we solve it. Likely, this is also the reason for ximion/appstream-generator#100 Thanks a lot for your time and your help! And sorry in the end the bug seems to be totally unrelated to the code you wrote, it feels like we are going to have a hard time figuring this one out :( EDIT: I also tried compiling latest master from the generator as suggested. In alpine, I still get the same results. |
|
@pabloyoyoista Don't worry :-) A bug is a bug. Which D compiler and which C compiler does Alpine use? This is an extremely odd bug... |
Yes, if at least it crashed...
We build And from I understand these are actually the C and D default compilers. Also, all of those link against |
|
If you also use GCC for the C code, then that's very uncontroversial, that would be what pretty much everyone uses - except for musl, of course. |
|
Well, it actually looks like the problem is in the C code-path: The |
|
That's what I expected. Still not sure what the cause of this issue is, but this will simplify debugging greatly :-) |
|
I've finally had the time and the energy to step by step dig into this with GDB, and it turns out the problem is not with musl, but with locales and maybe a bug in this project. What is currently happening in my alpine system with
A simple fix would be to use |
This helps #395, but the actual proper fix is to implement handling of screenshot localization in compose properly, as the current code is completely oblivious to that. In addition to that change, language compatibility checks for the C locales also don't seem to work as expected on Alpine, which needs further investigation as well.
|
Oh no! Thank you for the debugging effort! This is a direct cause of the localized-screenshots support having been bolted on at a later stage, with older code now doing the wrong thing... On its own that would be a bug, but there also appears to be a second issue here with AppStream not considering the "C" locale compatible with "C.UTF-8" on Alpine in In the meanwhile, you could maybe try the patch above, which, if I understand the problem correctly, should at least prevent the bad data generation for now (fixing the root cause will take a bit more effort though). |
|
That certainly looks like it will certainly fix the issue. I'll test it this weekend locally, and if it works, push the fix to alpine.
Is this something you would need us to debug too? Some specific information on what is the expected behavior and what is that the localization is messing up? |
|
And BTW, thanks a lot for the very fast fix!! :D |
|
I can confirm the issue is gone locally!! Will proceed to push it to alpine and to test it in the generator container. Thank you very, very much for your help!! |
|
I can confirm this is gone in the container after dropping the caches :) Feel free to close this if you consider it solved! |
Is there a new data generated somewhere already, to test? |
The full run is in process. You'll find it updated in my testing machine in a couple of hours: https://appstream.pcorreag.tk/data/ |
This needs two more things to be properly fixed :-) Not sure yet what the locale issue is in Alpine, but I'll add a unit test for that so we can investigate that further (and maybe doing so will also yield some insight already). |
|
This should work now, but keep an eye out if localization of AppStream data works well on Alpine clients! |
|
Wonderful, thank you very much! We'll see if we bump into more issues. But it looks like the code handles well the circumstance where there is no locale environment variable set. So it should be all fine :) |
Symptom: screenshot URLs look like this (note double
https://) (sreenshot from debugger session)As we can see in src/as-image.c#L342 base url is appended unconditinally, resulting in screenshot image that cannot be downloaded and degraded user experience in software center application (plasma-discover in our case).
Metadata XML fragment for this looks like:
The issue can be reproduced by small example program that loads single metadata file:
taken from real data
As you can see also from test example, first screenshot URL has extra
https://added.Suggested fix is by checking for absolute URL with g_str_has_prefix (or whatever the correct way in glib is, I'm Qt user, don't know much about glib). In case if screenshot URL is already starting with
https://, do not append media_baseurl.P.S. downstream issue for reference: https://gitlab.alpinelinux.org/alpine/infra/compose/appstream-generator/-/issues/2
The text was updated successfully, but these errors were encountered: