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

[WIP] Allow AppStream to parse recommendations #45

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@lucasmoura
Copy link
Contributor

lucasmoura commented Jun 2, 2016

This MR add the feature to AppStream to allow it to parse recommendation files. A recommendation file is defined by a XML document in the following format:

<?xml version="1.0"?>
    <recommendations>
        <recommendation>
            <id>vim.desktop</id>
            <because>
                <id>emacs.desktop</id>
            </because>
        </recommendation>
        <recommendation>
            <id>ipython.desktop</id>
            <because>
                <id>eric.desktop</id>
                <id>winpdb.desktop</id>
            </because>
        </recommendation>
    </recommendations>

Where id is the package being recommended and because is the user packages that generated this recommendation.

Currently, it is already working the construction and parsing of a valid XML recommendation, however there are some necessary improvements to be done:

  • There is a lot of code that equals the one on the core AppStream modules and a refactoring is necessary to fix that.
  • It is necessary to validate a recommendation object, because if the recommended package is not indexed by AppStream, the recommendation object should be invalid.
  • Add an appstreamcli function to parse and display the recommendations.
  • Validate the XML that represents a recommendation.

Therefore, this MR is manly to collect feedback from the AppStream community regarding this feature.

@lucasmoura lucasmoura force-pushed the lucasmoura:recommendation_file branch from 9917d7e to cf67059 Jun 2, 2016

@ximion

This comment has been minimized.

Copy link
Owner

ximion commented Jun 2, 2016

Nice! I'll be a bit busy the next days (KDE AppStore meeting and other things), but I think I can give it a quick review tonight. One obvious thing to resolve would be fixing the test issues ;-) (minor stuff, as far as I can see).
Some quick initial questions:

  1. Where is that recommendation data supposed to come from? Are upstream projects supposed to ship it in their metainfo file? Or doe distributions seed that stuff?

  2. Regarding the XML example:

<recommendations>
        <recommendation>
            <recommended>vim.desktop</recommended>
            <because>
                <pkg>emacs.desktop</pkg>
            </because>

You want to define a condition here, right? So, something like "if Vim present, suggest Emacs". The "because" should therefore probably be an "if". Also, I think the whole block is a bit strange... Why not put the recommendation into the component-block of the thing recommending it? So, something like:

<component type="desktop">
  <id>emacs.desktop</id>
  <recommendations>
    <recommendation>emacs.desktop</recommendation>
  </recommendations>
</component>

That would look far more simple to me, and would prevent the need for declaring logic in XML. Aside from that, your pkg tag is actually referring to an AppStream-ID (probably a leftover from when it was package-based).

I need to read the code this evening to fully understand the concept behind this and give some better feedback though ;-)

@lucasmoura

This comment has been minimized.

Copy link
Contributor Author

lucasmoura commented Jun 2, 2016

No problem. I will look why the build is failing. It is currently presenting this error:

gpg: no valid OpenPGP data found.var/lib/app-inf

But I will look it up to fix it.

  1. Originally, I thought that the recommendation application could generate the file and place it a common place for AppStream to parse the recommendations. Something like /usr/share/recommendations. But I still don't know if that is the best approach.

  2. I think I have not explained the because tag properly. The recommendation would not be something like if the user has emacs.desktop then recommends vim.desktop, it would be because the user already has emacs.desktop then recommends vim.desktop. The recommender application would be the one responsible for asserting which packages the user has and control this representation.

But you are right, the tag names provided by your XML file are way better than the ones I have provided. I will create an XML file with better names and update the code accordingly.

@ximion ximion force-pushed the ximion:master branch 12 times, most recently from 5f9c0b5 to 65f44e5 Jun 2, 2016

@ximion

This comment has been minimized.

Copy link
Owner

ximion commented Jun 3, 2016

Sidenote: I really want a reply function in the GitHub UI! Making replys manually is annoying...

gpg: no valid OpenPGP data found.var/lib/app-inf
But I will look it up to fix it.

No worries, that was a temporary failure due to the LLVM APT repo not being available. I took that as an argument to make the whole build process happen in a Docker container now, which allows us to be independent of the outdated Travis base system.
So, to fix this for you, just rebase on master.

The recommender application would be the one responsible for asserting which packages the user has and control this representation.

Okay, then why have this because information in the first place? Wouldn't simply dropping a list of recommended AppStream-IDs into some directory in /var/cache be more useful? (and have AS pick that up then)

@lucasmoura lucasmoura force-pushed the lucasmoura:recommendation_file branch from cf67059 to 754d218 Jun 3, 2016

@lucasmoura

This comment has been minimized.

Copy link
Contributor Author

lucasmoura commented Jun 3, 2016

Okay, I have applied the rebase and I will check again if there is another problem during the build process.

But about the because tag. Originally, I though that it would be good for an user to know which of his packages generated the one being recommended. It would made the recommendation more trustworthy, since the user would see the reason behind it. Therefore, I thought that the applications that would use this AppStream data could display this extra information for their users.

However, maybe on the context of AppStream this extra information is really not necessary or I can make it optional.

@ximion ximion force-pushed the ximion:master branch 8 times, most recently from 94fcca3 to 96f54e1 Jun 6, 2016

@lucasmoura lucasmoura force-pushed the lucasmoura:recommendation_file branch 3 times, most recently from ba89753 to 8e964d2 Jun 6, 2016

@lucasmoura

This comment has been minimized.

Copy link
Contributor Author

lucasmoura commented Jun 7, 2016

I have updated some of the XML tag names and also added a method to check if the recommendation object is valid. By valid, I mean that the package referenced by the id variable is a package indexed by AppStream.

@lucasmoura lucasmoura force-pushed the lucasmoura:recommendation_file branch from 50db210 to cd36dfc Jul 29, 2016

@lucasmoura

This comment has been minimized.

Copy link
Contributor Author

lucasmoura commented Jul 29, 2016

I have added the "merge" type for a component class. I have also made a verification on the
as_data_pool_merge_components method, to only merge components if the src_cpt has the merge type.

@ximion ximion force-pushed the ximion:master branch from 9dec939 to 18c0750 Aug 8, 2016

ximion added a commit that referenced this pull request Aug 9, 2016

typedef struct
{
AsSuggestedKind kind;
GPtrArray *cpts_id; /* of string */

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

Should rather be cpt_ids of utf8

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

On second thought, I also wonder if a GHashTable isn't a better idea here, since that will prevent us from having duplicate entries...

* Returns: a #AsSuggestedKind or %AS_SUGGESTED_KIND_UNKNOWN for unknown
**/
AsSuggestedKind
as_suggested_kind_from_string (const gchar *kind_str)

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

For a not-set string (NULL) we should assume UPSTREAM.

{
AsSuggestedPrivate *priv = GET_PRIVATE (suggested);

priv->cpts_id = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref);

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

This will explode, you don't want g_object_unref but g_free.

* Returns: an array of components id
*/
GPtrArray*
as_suggested_get_components_id (AsSuggested* suggested)

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

get_component_ids - it's multiple... (otherwise I would expect some kind of filter to just return one)

as_suggested_add_component_id (AsSuggested *suggested, gchar* cpt_id)
{
AsSuggestedPrivate *priv = GET_PRIVATE (suggested);
g_ptr_array_add (priv->cpts_id, cpt_id);

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

Uuuh, you need to strdup here, otherwise ownership of the pointer is transferred to the ptr-array, which will free it later. This isn't good.

* Returns: TRUE if the suggestion data was validated successfully.
*/
gboolean
as_suggested_is_valid (AsSuggested *suggested)

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

A method like this isn't necessary - but it also doesn't hurt, so let's have it :)

return FALSE;

if (priv->cpts_id->len != 0)
ret = TRUE;

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

Why so complicated with a variable? Just return TRUE and otherwise FALSE.

*
* Get a list of components id that generated the suggestion
*
* Returns: an array of components id

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

This part is missing GIR annotations.

AsSuggestedKind kind);

GPtrArray* as_suggested_get_components_id (AsSuggested *suggested);
void as_suggested_add_component_id (AsSuggested *suggested, gchar* cpt_id);

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

Should be a const gchar* as parameter.

* Add an #AsSuggested to this component.
**/
void
as_component_add_suggestion (AsComponent *cpt, AsSuggested* suggested)

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

Should likely be add_suggested for consistency.

{
GPtrArray* suggestions;

suggestions = as_component_get_suggestions (cpt);

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

No need for this, just access the variable directly.

@ximion

This comment has been minimized.

Copy link
Owner

ximion commented Aug 9, 2016

I fixed up the first two patches and committed the result to master.
Can you please rebase the remaining bits on top of latest master? (there were quite a few changes, for example the inclusion of the merge component, and a clean set of patches will be easier to review).

Bonus points for adding some validator hints (e.g. having a type=heuristic component in metainfo files is an error).

g_return_if_fail (xdt != NULL);
g_return_if_fail (cpt != NULL);

suggested = as_suggested_new ();

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

You are leaking memory here.

g_return_if_fail (cpt != NULL);

suggested = as_suggested_new ();
type_str = (gchar*) xmlGetProp (node, (xmlChar*) "type");

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

...and here (needs to be free'd)

node_name = (gchar*) iter->name;

if (g_strcmp0 (node_name, "id") == 0) {
content = as_xmldata_get_node_value (xdt, iter);

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

As well as here (actually, this is probably the biggest leak, since this needs to be freed on each iteration)

}

if (as_suggested_is_valid (suggested))
as_component_add_suggestion (cpt, suggested);

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

You are adding the AsSuggested again and again here, once per ID...

{
guint i;

g_autoptr(GPtrArray) cpts = NULL;

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

Unused variable - weird that the CI didn't complain about this...

<?xml version="1.0" encoding="UTF-8"?>
<components version="0.8" origin="jessie">
<component type="desktop">
<id>test.desktop</id>

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

Merge components need some component to merge their data into - otherwise they don't exist in the pool.
Since there is no test.desktop, this would fail.

cpts_id = as_suggested_get_components_id (suggested);
suggested_kind = as_suggested_get_kind (suggested);

expected_cpts_id = g_new0 (gchar*, 3);

This comment has been minimized.

@ximion

ximion Aug 9, 2016

Owner

Memory leak... You need to free the strv first.

@ximion

This comment has been minimized.

Copy link
Owner

ximion commented Aug 9, 2016

The contents of this PR are now merged - there is still work to do, mainly support for writing the suggests tag and for the YAML part is completely missing.
Also, there are no tests for reading the data from an XML file / writing it yet.

@ximion

This comment has been minimized.

Copy link
Owner

ximion commented Aug 9, 2016

Thanks for submitting this PR!

@ximion ximion closed this Aug 9, 2016

@lucasmoura

This comment has been minimized.

Copy link
Contributor Author

lucasmoura commented Aug 10, 2016

Sorry for that huge amount of memory leaks. I will be more careful on my next merge request. I think I will now allow AppRecommender to write the XML files and look into the YAML part as well.

Thanks for the patience on reviewing this feature :)

@ximion

This comment has been minimized.

Copy link
Owner

ximion commented Aug 10, 2016

I already took care of the YAML world (and pretty much all other bits) - the remaining piece is getting the merge logic work for (almost) all fields, see the as_merge_components function in AsDataPool.

I hope I can do a new AppStream release ASAP.
Please pay attention that @hughsie and I changed how component merges are done after a brief discussion on IRC, check the example file in the testsuite for how it's done now :)

The libappstream library is capable of writing the necessary XML or YAML files for merge components now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.