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

spec: New "cockpit-manifest" launchable type #127

Merged
merged 1 commit into from Aug 29, 2017

Conversation

mvollmer
Copy link
Contributor

This is used by the upcoming Cockpit "Software Center": cockpit-project/cockpit#7076

@ximion
Copy link
Owner

ximion commented Aug 17, 2017

I am usually against vendor/product-specific stuff in AppStream, so, convince me!

  • Can this be declared in a non Cockpit-specific manner in AppStream? (e.g. by putting an URL there)
  • Is this useful (= implementable) for other projects as well in any way? Is someone else using it?
  • How is this Launch-Type intended to be represented in UIs? Does Cockipit need to be installed for it to be usable at all?
  • How many users would this have?

I'll need to read the linked documentation on what a cockpit-package is (so far, I just quickly skimmed through it).
Also, @hughsie feedback is welcome!

@hughsie
Copy link
Collaborator

hughsie commented Aug 18, 2017

I'm really not sure about the suffix -package -- to me a package is a .rpm, .deb etc. I think it's fine to have project specific components, e.g. I'm already using (probably incorrectly) shell-extension for gnome-shell extensions. I think it's fair to say that a cockpit component type is basically useless to anything except cockpit, right?

@mvollmer
Copy link
Contributor Author

mvollmer commented Aug 18, 2017

I am usually against vendor/product-specific stuff in AppStream

That is a fair objection, of course.

If you squint hard enough, you can see Cockpit as a type of desktop shell that launches applications within itself. Traditionally, all applications were coming from the Cockpit project itself, but we want to open this up.

The equivalent of Cockpit to /usr/share/applications/foo.desktop is /usr/share/cockpit/foo/manifest.json. This manifest file can contribute to Cockpits navigation menus.

Here is an example: https://github.com/mvollmer/cockpit-app-freeipa/blob/master/manifest.json

This example puts a new "FreeIPA" entry into the Cockpit navigation, and clicking on that entry will load /usr/share/cockpit/freeipa/index.html into the browser (within an iframe).

If there would be competing versions of Cockpit-like desktops (like there are GNOME, KDE, ...), we would have to make this manifest mechanism Cockpit-independent, and it would be almost exactly like the desktop-file situation, except for 'browser based desktops'.

Can this be declared in a non Cockpit-specific manner in AppStream? (e.g. by putting an URL there)

I am not sure I understand. We can't use any of the existing <launchable> types, no? Are you proposing to not use <launchable> tags at all?

Maybe we can encode it like this:

<component type="addon">
  <extends>cockpit.desktop</extends>
  <launchable type="addon">app-freeipa</launchable>
</component>

Thus, a component of type "addon" can contain launchables of type "addon" and what they mean is determined by the thing that is being extended.

Is this useful (= implementable) for other projects as well in any way? Is someone else using it?

It would be implementable by others, but I haven't tried to find any other 'browser-based desktops'.
Installers and image composers might look at <launchable type="cockpit-package"> tags, maybe, although they might be better off with looking at add-ons for Cockpit.

How is this Launch-Type intended to be represented in UIs? Does Cockipit need to be installed for it to be usable at all?

Yes, this only makes sense with Cockpit. All components with a <launchable type="cockpit-package"> tag will very likely be type=add-on for "cockpit.deskop".

How many users would this have?

"Users" == components that will include a <launchable type="cockpit-package"> tag?

I know of two, right now. :-) Hard to predict how many there will be.

@mvollmer
Copy link
Contributor Author

I'm really not sure about the suffix -package -- to me a package is a .rpm, .deb etc.

Yeah. <launchable type="cockpit-manifest">?

I think it's fine to have project specific components, e.g. I'm already using (probably incorrectly) shell-extension for gnome-shell extensions.

But it's a different thing to put them in the spec, no?

Will things like appstream-util need to be updated before we can use a new launchable type end-to-end?

I think it's fair to say that a cockpit component type is basically useless to anything except cockpit, right?

Yes. We will use <component type="addon"> with <extends>cockpit.desktop</extends>.

@ximion
Copy link
Owner

ximion commented Aug 18, 2017

@hughsie

I think it's fine to have project specific components, e.g. I'm already using (probably incorrectly) shell-extension for gnome-shell extensions. I think it's fair to say that a cockpit component type is basically useless to anything except cockpit, right?

I would have rejected a new component type, as I want to keep the amount of different component types as small as possible. Having addons extending a base is a far more powerful method. Otherwise, as soon as I allow one, I would need to allow new types for a lot of projects, which isn't manageable. That being said, your shell extensions should be addons to the org.gnome.Shell component ;-)
Fortunately, this request is about a launchable-type, where I have a less strong opinion about.

@mvollmer Thank you for your detailed explanation! I actually think adding this does make sense here, and I also agree with hughsie that the "package" naming is a bit odd. I like your <launchable type="cockpit-manifest"> suggestion, would you be fine with that?

@mvollmer
Copy link
Contributor Author

I like your suggestion, would you be fine with that?

Yes, of course. I'll make that change.

@mvollmer mvollmer changed the title spec: New "cockpit-package" launchable type spec: New "cockpit-manifest" launchable type Aug 21, 2017
@mvollmer
Copy link
Contributor Author

@ximion, do you think we can go ahead and start using <launchable type="cockpit-manifest"/>?

ximion
ximion approved these changes Aug 28, 2017
Copy link
Owner

@ximion ximion left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the lack of feedback - I was busy converting the codebase to the Meson buildsystem, and I found a stunning amount of bugs or weird behavior while doing so... (most of them are fixed now, a few PRs for Meson are pending).
Since I don't merge changes in the spec without having the implementation also ready quickly afterwards, I delayed this PR (the implementation here consists of a simple four-line patch though ^^)

I added a few minor nitpicks (an example would be nice, a hyperlink to the project page would be nice as well).
I'll probably merge this tomorrow.

<listitem>
<para>
The software can be launched from the menus of
the Cockpit admin interface. The value of the
Copy link
Owner

Choose a reason for hiding this comment

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

Could you link "Cockpit" to the project page, so everyone can quickly look up which project this is about?

The manifest of the package is expected to
contain at least one entry for the Cockpit
shell menus.
</para>
Copy link
Owner

Choose a reason for hiding this comment

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

Can you maybe add an example of how this looks like? (So it's quick to find out what the "name of a cockpit package" is to insert.)

Copy link
Contributor Author

@mvollmer mvollmer Aug 29, 2017

Choose a reason for hiding this comment

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

I think this is already explained in the linked Cockpit documentation, no?

I think we can actually remove this paragraph completely and trust people to do the right thing. Cockpit currently expects a menu entry named "index", but that's just a implementation limitation that we should lift.

Copy link
Owner

Choose a reason for hiding this comment

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

I was quickly looking through the docs and it wasn't immediately clear to me what the "package name" is actually derived from. But it turns out the "Package Manifest" section is pretty clear about it.
Let's go without example then (if one goes crazy with examples, reading the docs will also become annoying, just like when there are too few examples).

@ximion
Copy link
Owner

ximion commented Aug 29, 2017

Oh, and btw, to answer your question: I think it's safe to use this - but ideally wait until it's implemented in AppStream, so people have a chance to get a working validator from somewhere :-)

@ximion ximion merged commit 6c49536 into ximion:master Aug 29, 2017
@mvollmer
Copy link
Contributor Author

Will things like appstream-util need to be updated before we can use a new launchable type end-to-end?

Turns out, yes. appstream-builder throws away unknown type attributes. It leaves the <launchable> tag and its children in place, it just removes the type attribute. I guess this just means some extra clerical work. I try to prepare a pull request for appstream-builder.

@hughsie
Copy link
Collaborator

hughsie commented Sep 14, 2017

I think it converts them to enums as part of the parsing process. Thanks for the patch tho :)

@ximion
Copy link
Owner

ximion commented Sep 14, 2017

Great :-)

@mvollmer
Copy link
Contributor Author

It leaves the tag and its children in place, it just removes the type attribute.

(This is not correct, I got confused by looking at appstream-builder debugging output. The whole <launchable> tag is removed in the final output. I shut up now.)

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

Successfully merging this pull request may close these issues.

None yet

3 participants