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

Building in chroot broken by AppStream verification #306

Closed
mikhailnov opened this issue Aug 21, 2018 · 17 comments
Closed

Building in chroot broken by AppStream verification #306

mikhailnov opened this issue Aug 21, 2018 · 17 comments

Comments

@mikhailnov
Copy link
Contributor

/usr/bin/appstream-util validate-relax data/com.github.wwmm.pulseeffects.appdata.xml
--- stdout ---
data/com.github.wwmm.pulseeffects.appdata.xml: FAILED:
• url-not-found         : <screenshot> failed to connect: Cannot resolve hostname [https://raw.githubusercontent.com/wwmm/pulseeffects/master/images/appdata-screenshot-01.png]
• url-not-found         : <screenshot> failed to connect: Cannot resolve hostname [https://raw.githubusercontent.com/wwmm/pulseeffects/master/images/appdata-screenshot-05.png]
• url-not-found         : <screenshot> failed to connect: Cannot resolve hostname [https://raw.githubusercontent.com/wwmm/pulseeffects/master/images/appdata-screenshot-04.png]
• url-not-found         : <screenshot> failed to connect: Cannot resolve hostname [https://raw.githubusercontent.com/wwmm/pulseeffects/master/images/appdata-screenshot-02.png]
• url-not-found         : <screenshot> failed to connect: Cannot resolve hostname [https://raw.githubusercontent.com/wwmm/pulseeffects/master/images/appdata-screenshot-03.png]
--- stderr ---

https://launchpadlibrarian.net/384677539/buildlog_ubuntu-bionic-amd64.pulseeffects_4.2.8-1~bionic1_BUILDING.txt.gz

@AsavarTzeth

mikhailnov added a commit to mikhailnov/pulseeffects that referenced this issue Aug 21, 2018
@mikhailnov mikhailnov mentioned this issue Aug 21, 2018
@mikhailnov
Copy link
Contributor Author

Fixed it by adding --nonet.

@AsavarTzeth
Copy link
Contributor

AsavarTzeth commented Aug 22, 2018

Well you could just not build the test target? Or is Debian tools somehow enforcing this globally? I mean at the moment these are the only tests run in that target and they are mostly useful to upstream anyhow.

@mikhailnov
Copy link
Contributor Author

mikhailnov commented Aug 22, 2018 via email

@AsavarTzeth
Copy link
Contributor

AsavarTzeth commented Aug 22, 2018

Oh I agree that its a good idea, but in this specific case these are the only tests. So here it would work as a temporary workaround.

@mikhailnov
Copy link
Contributor Author

mikhailnov commented Aug 22, 2018 via email

@AsavarTzeth
Copy link
Contributor

To validate the size and format of the screenshots so that they can displayed properly in frontends such as GNOME Software.

@AsavarTzeth
Copy link
Contributor

Basically, a frontend will pull in images from these URL's and unless verified, there is no way for the frontend to know if it will work or look right.

@AsavarTzeth
Copy link
Contributor

AsavarTzeth commented Aug 22, 2018

Although, now that I think about it, these screenshots have so far only ever been updated by me and I do test them well.

If --nonet only affects screenshots I suppose it would not be as big of a deal if those were not tested at build time. It just means it will have to be verified manually upon updates.

The man page isn't too clear on what tests --nonet disables. I will try to dig some more to find out.

@wwmm
Copy link
Owner

wwmm commented Aug 22, 2018

At some point it crossed my mind to update these appdata screenshots. But I was not sure if there was some restriction regarding quality and size. At this moment we have images shown in github readme and images used only in appstream. We could unify this.

@AsavarTzeth
Copy link
Contributor

I agree they need to be updated and ideally unified. And yes there are quite a list of restrictions that are horrible to work with.

I have more questions on that issue. Perhaps I should open an issue about unifying the screenshots? I could add a lot of information and maybe a todo list?

@wwmm
Copy link
Owner

wwmm commented Aug 22, 2018

If there are more details to be discussed about the screenshots it is better to open another issue

@mikhailnov
Copy link
Contributor Author

Why can't the screenshots be taken from source code instead of downloading them from GitHub?

@wwmm
Copy link
Owner

wwmm commented Aug 23, 2018

good question

@AsavarTzeth
Copy link
Contributor

AsavarTzeth commented Aug 23, 2018

What? I fail to see your reasoning.

The screenshots are stored in the source code, on GitHub. They have to be hosted somewhere, and downloadable from a web server.

@AsavarTzeth
Copy link
Contributor

AsavarTzeth commented Aug 23, 2018

Oh you mean for the verification process? Well think about what I said above.

  • Checking that the link is active and working is part of the validation.
  • Screenshots are served via http/https. We basically use GitHub as a web server. That could change if Flathub introduces screenshot storage.

You could theoretically verify just the screenshots offline, but that would require what? Some weird tag in the appdata that describes a relative path to the image? It also offers no guarantee to frontends that image actually will be used.

Edit: I would so like a do over on this question :)

@wwmm
Copy link
Owner

wwmm commented Aug 23, 2018

If that is how it works the online validation makes sense.

@AsavarTzeth
Copy link
Contributor

AsavarTzeth commented Aug 23, 2018

Haha glad I somehow got something through!

Side note: I am guessing the short answer would have been: Because those URL's are what frontends actually use to download and display screenshots? The rest is then apparent.

AsavarTzeth pushed a commit to AsavarTzeth/pulseeffects that referenced this issue Aug 24, 2018
AsavarTzeth pushed a commit to AsavarTzeth/pulseeffects that referenced this issue Aug 24, 2018
AsavarTzeth pushed a commit to AsavarTzeth/pulseeffects that referenced this issue Aug 24, 2018
AsavarTzeth pushed a commit to AsavarTzeth/pulseeffects that referenced this issue Aug 24, 2018
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 a pull request may close this issue.

3 participants