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

org.virt_manager.virt-viewer #391

Closed
wants to merge 24 commits into from
Closed

org.virt_manager.virt-viewer #391

wants to merge 24 commits into from

Conversation

AdrianKoshka
Copy link

This is a rough-draft PR, I still need to write an appstream file, but mostly everything else seems done.

@@ -0,0 +1,60 @@
<!-- https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html -->
Copy link
Member

Choose a reason for hiding this comment

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

Have you submitted this upstream?

Copy link
Author

Choose a reason for hiding this comment

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

Not yet, I'm waiting for upstream to reply to an email I sent to their mailing-list. I'll probably submit this appstream rough-draft to upstream in a following email.

"sources": [
{
"type": "git",
"url": "git://anongit.freedesktop.org/spice/spice-protocol",
Copy link
Member

Choose a reason for hiding this comment

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

Use https here.

Copy link
Author

Choose a reason for hiding this comment

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

Will-do

@nedrichards
Copy link
Member

bot, build org.virt_manager.virt-viewer

@barthalion
Copy link
Member

Filename doesn't match app ID.

@AdrianKoshka
Copy link
Author

I had been building only remote-viewer before because I didn't know I also needed to build libvirt to build virt-viewer, I'm working on fixing this now. what I intend to happen is when you run flatpak run org.virt_manager.virt-viewer, virt-viewer is ran, but if you run the .desktop of remote-viewer, then remote-viewer is ran.

@AdrianKoshka
Copy link
Author

I put the wrong https link for that git repo, will push a fix for that too, as soon as I get libvirt to build.

@barthalion
Copy link
Member

Take my comment literally. id is org.virt_manager.virt-viewer, while the file is named org.virt-manger.virt-viewer.json (note the typo). It's not going to working this way.

@AdrianKoshka
Copy link
Author

Oh, thanks, I'll fix this.

@AdrianKoshka
Copy link
Author

AdrianKoshka commented May 14, 2018

This still needs some serious cleanup, but it builds correctly now.

</categories>
<url type="homepage">https://virt-manager.org/download/</url>
<url type="bugtracker">https://bugzilla.redhat.com/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&classification=Community&component=virt-viewer&product=Virtualization%20Tools</url>
<url type="help">https://support.mozilla.org/questions/thunderbird</url>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong help URL.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching this, I wasn't really able to find a good "help" URL, so I'll just remove that line. Is that okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.

@@ -0,0 +1,60 @@
<!-- https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html -->
<?xml version="1.0" encoding="UTF-8"?>
<component type="desktop">
Copy link
Contributor

Choose a reason for hiding this comment

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

No screenshots.

Copy link
Author

Choose a reason for hiding this comment

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

What resolution should screenshots be? As a reference.

"name": "lz4",
"build-options": {
"env": {
"PREFIX": "/app"
Copy link
Contributor

Choose a reason for hiding this comment

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

You use ${FLATPAK_DEST} above.

Copy link
Author

Choose a reason for hiding this comment

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

So are you saying to replace PREFIX with FLATPAK_DEST?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, see module gcrypt-wrapper. I meant: Use /app or ${FLATPAK_DEST} consistently.

Copy link
Author

Choose a reason for hiding this comment

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

does 72a6936 solve this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Not that it does have any effect, it was only a minor style thing.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, you've been doing a great job of helping me better my manifest.

},
"buildsystem": "simple",
"build-commands": [
"make",
Copy link
Contributor

Choose a reason for hiding this comment

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

make -j $FLATPAK_BUILDER_N_JOBS

"*"
],
"build-commands": [
"install -Dm755 libgcrypt-config ${FLATPAK_DEST}/bin/libgcrypt-config"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be shortened to: install libgcrypt-config ${FLATPAK_DEST}/bin

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, that was wrong. You could only drop the "m755".

<categories>Network</categories>
</categories>
<url type="homepage">https://virt-manager.org/download/</url>
<url type="bugtracker">https://bugzilla.redhat.com/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&classification=Community&component=virt-viewer&product=Virtualization%20Tools</url>
Copy link
Contributor

Choose a reason for hiding this comment

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

$ appstream-util validate-relax org.virt_manger.virt-viewer.appdata.xml 
org.virt_manger.virt-viewer.appdata.xml: failed to parse org.virt_manger.virt-viewer.appdata.xml: Error on line 16: Entity did not end with a semicolon; most likely you used an ampersand character without intending to start an entity - escape ampersand as &amp;

@nedrichards
Copy link
Member

bot, build bot, build org.virt_manager.virt-viewer

@AdrianKoshka
Copy link
Author

So now I've gotten virt-viewer in a working state, the next problem I need to tackle is getting virt-viewer to connect to qemu:///system. qemu:///system is the URI for qemu/libvirt running on that local machine.

I think this would need to be done by passing a file through to the sandbox via --filesystem= (because virt-viewer looks for a file which is a socket). I'll also start working on bettering the appstream file.

@nedrichards
Copy link
Member

bot, build org.virt_manager.virt-viewer

@barthalion barthalion added the work-in-progress Draft PRs label May 19, 2018
@AdrianKoshka
Copy link
Author

Sorry for the radio silence, I was on vacation and went to an event over the past weekend.

@nedrichards
Copy link
Member

No worries at all! "Having A Life" is not a problem, when you're donating your time for free you don't owe anybody anything (unless you want to).

@AdrianKoshka
Copy link
Author

AdrianKoshka commented May 23, 2018

I don't think I'll be able to make USB redirection work, mostly because almost all of /dev is read-only.

  • All mounts are read-only, except:
    • root tmpfs (e.g. for tmp subdir and transient home)
    • /var
    • /proc (except /proc/sys, proc/sysrq-trigger, /proc/irq and /proc/bus which are
      read-only)
    • /dev/pts
    • /dev/shm

Another thing I haven't worked out is how to connect to qemu:///system, which is qemu(kvm)/libvirtd running on the host. virt-viewer seemingly needs /var/run/libvirt/libvirt-sock-ro.

Also a way to pass the SSH-agent of the host through to the flatpak.

@nedrichards
Copy link
Member

bot, build org.virt_manager.virt-viewer

@AdrianKoshka
Copy link
Author

As long as the appstream data is fine, I think I'm fine with merging this in it's current state. I think more people will use remote-viewer anyway (which mostly works, sans usb-redirection, but boxes also has this problem).

"sources": [
{
"type": "archive",
"url": "https://github.com/gentoo/eudev/archive/v3.2.5.tar.gz",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I think the version in shared modules was out of date for my needs.

"sources": [
{
"type": "archive",
"url": "https://libvirt.org/sources/libvirt-4.3.0.tar.xz",
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it matters for app functionality, but 4.4.0 is o ut.

Copy link
Author

Choose a reason for hiding this comment

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

Updated it (and a few other things) anyway.

"--talk-name=org.a11y.*",
"--device=all",
"--filesystem=xdg-pictures",
"--filesystem=~/.ssh",
Copy link
Member

Choose a reason for hiding this comment

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

There is also --socket=ssh-auth now, but I'm not sure if it's enough for ssh to work.

Copy link
Author

Choose a reason for hiding this comment

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

I plan on testing this tonight.

Copy link
Author

Choose a reason for hiding this comment

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

I'm also thinking of adding dri, becasue spice can leverage openGL for rendering.

Copy link
Author

Choose a reason for hiding this comment

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

So, while the ssh-auth socket is enough for ssh to work on a basic level, if ~/.ssh isn't passed through, it seemingly doesn't create a known_hosts file, and will always prompt the user whether they trust the fingerprint or not.

@nedrichards
Copy link
Member

bot, build org.virt_manager.virt-viewer

@barthalion
Copy link
Member

JSON manifest has typo in the filename, it h=should be org.virt_manager.virt-viewer, not manger.

@AdrianKoshka
Copy link
Author

Thanks.

@barthalion
Copy link
Member

bot, build org.virt_manager.virt-viewer

Makefile Outdated
@@ -0,0 +1,18 @@
TARGET_REPO = repo
FLATPAK_BUILDER = $(shell which flatpak-builder)
MANIFEST = org.virt_manger.virt-viewer.json
Copy link
Member

Choose a reason for hiding this comment

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

Here is also a typo. I'd rather remove Makefile completely.

@@ -0,0 +1,74 @@
<!-- https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html -->
Copy link
Member

Choose a reason for hiding this comment

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

You are not installing this file anywhere. Have you submitted it upstream too?

Copy link
Author

Choose a reason for hiding this comment

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

Haven't yet, the last time I tried to contact upstream I got absolutely nothing back.

@nedrichards
Copy link
Member

bot, build org.virt_manager.virt-viewer

@nedrichards
Copy link
Member

bot, build org.virt_manager.virt-viewer

@AdrianKoshka
Copy link
Author

Not happy with how this flatpak operates. Mostly do USB redirection not working. I don't think I'll be submitting it after-all. I guess if anyone wants to pick it up in future, I could help them though. Thanks for all the help, apologies for the wasted effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants