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

Peer to peer and unsigned summary file support #884

Merged
merged 40 commits into from Aug 18, 2017

Conversation

Projects
None yet
5 participants
@pwithnall
Collaborator

pwithnall commented Jun 30, 2017

Here’s my branch which adds support for installing/updating apps/runtimes from LAN/USB, including the necessary support for unsigned summary files and the ostree-metadata branch (see ostreedev/ostree#983).

It adds a new --enable-p2p option to hide the new functionality behind, but most of the commits have not been updated to actually conditionalise their changes on that option yet. Thus this branch is ready for initial review, but should not be merged yet. I’ll update it soon to add the #ifdefs, but they shouldn’t impact the overall behaviour or structure of the branch much.

There are also a bunch of TODO comments in the final commit which indicate the things I still need to check or implement (they will mostly be corner cases or upstreaming things to libostree; see below).

It adds a couple of new tests, but there are bound to be quite a few corner cases which I haven’t handled yet. I plan to test it for a while with --enable-p2p before we consider dropping the option.

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Jun 30, 2017

Collaborator

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once
Collaborator

rh-atomic-bot commented Jun 30, 2017

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once
context = g_main_context_new ();
g_main_context_push_thread_default (context);
ostree_repo_find_remotes_async (self->repo, (const OstreeCollectionRef * const *) collection_refs_to_fetch,

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

Surely you need some ifdefs here.

@alexlarsson

alexlarsson Jun 30, 2017

Member

Surely you need some ifdefs here.

This comment has been minimized.

@pwithnall

pwithnall Jul 3, 2017

Collaborator

Yeah, a lot of the necessary #ifdefs are missing in this WIP version of the branch.

@pwithnall

pwithnall Jul 3, 2017

Collaborator

Yeah, a lot of the necessary #ifdefs are missing in this WIP version of the branch.

This comment has been minimized.

@pwithnall

pwithnall Aug 4, 2017

Collaborator

(Since fixed.)

@pwithnall

pwithnall Aug 4, 2017

Collaborator

(Since fixed.)

Show outdated Hide outdated app/flatpak-builtins-build-export.c
@@ -652,7 +653,7 @@ flatpak_builtin_build_export (int argc, char **argv, GCancellable *cancellable,
guint64 installed_size = 0,download_size = 0;
GTimeVal ts;
context = g_option_context_new (_("LOCATION DIRECTORY [BRANCH] - Create a repository from a build directory"));
context = g_option_context_new (_("LOCATION DIRECTORY [[COLLECTION-ID] BRANCH] - Create a repository from a build directory"));

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

Not sure about the order here. You might want to have a branch, but not set a collection id, whereas there is always a branchname, even if you leave the argument out.

In fact, isn't it better if this is --collection-id=ID

@alexlarsson

alexlarsson Jun 30, 2017

Member

Not sure about the order here. You might want to have a branch, but not set a collection id, whereas there is always a branchname, even if you leave the argument out.

In fact, isn't it better if this is --collection-id=ID

This comment has been minimized.

@pwithnall

pwithnall Jul 3, 2017

Collaborator

Not sure about the order here. You might want to have a branch, but not set a collection id, whereas there is always a branchname, even if you leave the argument out.

Those cases can be differentiated by looking at argc, but I agree that it’s a bit confusing. I put the collection ID before the branch name because it’s always presented before the branch name elsewhere — i.e. like in the tuple ($collection_id, $ref_name).

In fact, isn't it better if this is --collection-id=ID

I decided against that because ideally most flatpaks are (eventually) going to have a collection ID set, so it would ideally be a positional argument. However, --collection-id is probably a better solution to prevent the weirdness with argc and ordering. I’ll change it.

@pwithnall

pwithnall Jul 3, 2017

Collaborator

Not sure about the order here. You might want to have a branch, but not set a collection id, whereas there is always a branchname, even if you leave the argument out.

Those cases can be differentiated by looking at argc, but I agree that it’s a bit confusing. I put the collection ID before the branch name because it’s always presented before the branch name elsewhere — i.e. like in the tuple ($collection_id, $ref_name).

In fact, isn't it better if this is --collection-id=ID

I decided against that because ideally most flatpaks are (eventually) going to have a collection ID set, so it would ideally be a positional argument. However, --collection-id is probably a better solution to prevent the weirdness with argc and ordering. I’ll change it.

Show outdated Hide outdated app/flatpak-builtins-build-export.c
@@ -836,6 +866,9 @@ flatpak_builtin_build_export (int argc, char **argv, GCancellable *cancellable,
if (!flatpak_repo_collect_sizes (repo, root, &installed_size, &download_size, cancellable, error))
goto out;
g_variant_dict_insert_value (&metadata_dict, "ostree.commit.collection-ref",
g_variant_new ("(ss)", (collection_id != NULL) ? collection_id : "", full_branch));

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

Shouldn't we inherit the current repo collection id if there is one rather than NULL?

@alexlarsson

alexlarsson Jun 30, 2017

Member

Shouldn't we inherit the current repo collection id if there is one rather than NULL?

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 30, 2017

Member

Don't we need a "collection id" property on FlatpakRemote in libflatpak.

Member

alexlarsson commented Jun 30, 2017

Don't we need a "collection id" property on FlatpakRemote in libflatpak.

Show outdated Hide outdated common/flatpak-dir.c
@@ -1929,6 +1932,7 @@ repo_pull_one_dir (OstreeRepo *self,
collection_refs_to_fetch[0] = &collection_ref;
collection_refs_to_fetch[1] = NULL;
/* TODO: This implies the summary is downloaded elsewhere. Is this option actually accepted? */

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

Yes, we already did this when looking for updates. So we avoid ensure we get exactly the same version we detected then, as various decisions (like permissons) may have been done based on the metadata for that commit.

@alexlarsson

alexlarsson Jun 30, 2017

Member

Yes, we already did this when looking for updates. So we avoid ensure we get exactly the same version we detected then, as various decisions (like permissons) may have been done based on the metadata for that commit.

Show outdated Hide outdated common/flatpak-dir.c
@@ -8886,6 +8914,7 @@ flatpak_dir_update_remote_configuration (FlatpakDir *self,
return flatpak_dir_update_remote_configuration_for_repo_metadata (self, remote, summary, FALSE, NULL, cancellable, error);
}
/* TODO: Move these sizes out to per-ref or per-commit metadata */

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

This comment scares me a bit. We don't want to be in the situation where we have gnome-software blocking on startup until it has downloaded all the commit objects in all remotes before it starts showing anything, that will be very slow.

We already store this info in the commit metadata, but we propagate it to the summary for this reason. The summary file is a single, reasonlably compact file that we can download once and the have all info we need.

This, in combination with using unsigned summaries implies that we have to keep track of what the summary metadata said about a specific revision, and then verify that it was actually true when we pull the commit. I.e. we should not only verify that the signed commit has the same ref and collection id, but also that the metadata is identical to what was in the summary.

@alexlarsson

alexlarsson Jun 30, 2017

Member

This comment scares me a bit. We don't want to be in the situation where we have gnome-software blocking on startup until it has downloaded all the commit objects in all remotes before it starts showing anything, that will be very slow.

We already store this info in the commit metadata, but we propagate it to the summary for this reason. The summary file is a single, reasonlably compact file that we can download once and the have all info we need.

This, in combination with using unsigned summaries implies that we have to keep track of what the summary metadata said about a specific revision, and then verify that it was actually true when we pull the commit. I.e. we should not only verify that the signed commit has the same ref and collection id, but also that the metadata is identical to what was in the summary.

This comment has been minimized.

@pwithnall

pwithnall Jul 3, 2017

Collaborator

This comment scares me a bit. We don't want to be in the situation where we have gnome-software blocking on startup until it has downloaded all the commit objects in all remotes before it starts showing anything, that will be very slow.

The comment’s not very well worded, and it was more of a note to myself than to others. It’s to remind me to look into moving the entries from xa.cache into the additional metadata for each ref in the summary file (i.e. the a{sv} in the 0th child of OSTREE_SUMMARY_GVARIANT_FORMAT) so that the xa.cache property itself is split up and eliminated.

The bit about ‘per-commit metadata’ is just wrong.

@pwithnall

pwithnall Jul 3, 2017

Collaborator

This comment scares me a bit. We don't want to be in the situation where we have gnome-software blocking on startup until it has downloaded all the commit objects in all remotes before it starts showing anything, that will be very slow.

The comment’s not very well worded, and it was more of a note to myself than to others. It’s to remind me to look into moving the entries from xa.cache into the additional metadata for each ref in the summary file (i.e. the a{sv} in the 0th child of OSTREE_SUMMARY_GVARIANT_FORMAT) so that the xa.cache property itself is split up and eliminated.

The bit about ‘per-commit metadata’ is just wrong.

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 30, 2017

Member

So, i had a quick look at tihs code, and overall it looks reasonable.
I have two worries based on the comments and the code.

First of all (as i wrote in the comment) is that we want to continue relying on just downloading the summary file, and then have all the data we need for e.g command line completion, automatic dependency resolution (i.e. know what runtime and extensions are affected when installing/updating), etc. We don't want to actually download all the commits in the repo.

Secondly, I worry a bit about all the mentionings of things being racy in the dynamic case. We do a search for revs at one point, surely we remember then also which rev came from where in the results we carry forward? We don't ask some randomly picked remote about some rev that some other remote said it had?

Member

alexlarsson commented Jun 30, 2017

So, i had a quick look at tihs code, and overall it looks reasonable.
I have two worries based on the comments and the code.

First of all (as i wrote in the comment) is that we want to continue relying on just downloading the summary file, and then have all the data we need for e.g command line completion, automatic dependency resolution (i.e. know what runtime and extensions are affected when installing/updating), etc. We don't want to actually download all the commits in the repo.

Secondly, I worry a bit about all the mentionings of things being racy in the dynamic case. We do a search for revs at one point, surely we remember then also which rev came from where in the results we carry forward? We don't ask some randomly picked remote about some rev that some other remote said it had?

@pwithnall

This comment has been minimized.

Show comment
Hide comment
@pwithnall

pwithnall Jul 3, 2017

Collaborator

Don't we need a "collection id" property on FlatpakRemote in libflatpak.

I did originally have a commit which added one, but then I realised the new API was never actually used, so I dropped the commit so that the API diff was minimised. I was thinking it could be added later once someone actually needs it.

(Adding public API which is ‘experimental’ turns into a large amount of pain in terms of advertising it in the pkg-config file, guarding it in the headers correctly for internal and potential external use, conditionally putting it in the documentation, etc.)

Collaborator

pwithnall commented Jul 3, 2017

Don't we need a "collection id" property on FlatpakRemote in libflatpak.

I did originally have a commit which added one, but then I realised the new API was never actually used, so I dropped the commit so that the API diff was minimised. I was thinking it could be added later once someone actually needs it.

(Adding public API which is ‘experimental’ turns into a large amount of pain in terms of advertising it in the pkg-config file, guarding it in the headers correctly for internal and potential external use, conditionally putting it in the documentation, etc.)

@pwithnall

This comment has been minimized.

Show comment
Hide comment
@pwithnall

pwithnall Jul 3, 2017

Collaborator

First of all (as i wrote in the comment) is that we want to continue relying on just downloading the summary file, and then have all the data we need for e.g command line completion, automatic dependency resolution (i.e. know what runtime and extensions are affected when installing/updating), etc. We don't want to actually download all the commits in the repo.

This branch does continue to rely on the metadata from the summary file (or, equivalently, the head commit in the ostree-metadata branch) and shouldn’t download all the commits in the repo. The comment which made you worry about this was poorly worded, sorry (see above).

Secondly, I worry a bit about all the mentionings of things being racy in the dynamic case. We do a search for revs at one point, surely we remember then also which rev came from where in the results we carry forward? We don't ask some randomly picked remote about some rev that some other remote said it had?

The OstreeRepoFinderResult **results array does this for us — it specifies which refs are available from which peers, and the checksums for each of those refs. The comments you’re referring to predate when I modified check_for_updates() to propagate the OstreeRepoFinderResult **results array, which should do what we want to eliminate races. However, the comments are still partially valid, because there is some cleanup and verification I can do on a few code paths to check the results from check_for_updates() are actually used and there are actually no races. I’ll do that before I drop the comment.

Collaborator

pwithnall commented Jul 3, 2017

First of all (as i wrote in the comment) is that we want to continue relying on just downloading the summary file, and then have all the data we need for e.g command line completion, automatic dependency resolution (i.e. know what runtime and extensions are affected when installing/updating), etc. We don't want to actually download all the commits in the repo.

This branch does continue to rely on the metadata from the summary file (or, equivalently, the head commit in the ostree-metadata branch) and shouldn’t download all the commits in the repo. The comment which made you worry about this was poorly worded, sorry (see above).

Secondly, I worry a bit about all the mentionings of things being racy in the dynamic case. We do a search for revs at one point, surely we remember then also which rev came from where in the results we carry forward? We don't ask some randomly picked remote about some rev that some other remote said it had?

The OstreeRepoFinderResult **results array does this for us — it specifies which refs are available from which peers, and the checksums for each of those refs. The comments you’re referring to predate when I modified check_for_updates() to propagate the OstreeRepoFinderResult **results array, which should do what we want to eliminate races. However, the comments are still partially valid, because there is some cleanup and verification I can do on a few code paths to check the results from check_for_updates() are actually used and there are actually no races. I’ll do that before I drop the comment.

pwithnall added a commit to pwithnall/flatpak that referenced this pull request Jul 4, 2017

common/dir: Drop unused variable
This was accidentally introduced in a8ad392 in advance of the LAN/USB
changes from PR #884 which will actually use it.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
@pwithnall

This comment has been minimized.

Show comment
Hide comment
@pwithnall

pwithnall Jul 7, 2017

Collaborator

I’ve just force-pushed an updated version of the branch, rebased on master and with quite a few fixes squashed in. It should now have all the necessary #ifdefs. There’s one niggle about conditional enum parsing for glib-mkenums which I need to solve at some point, but this is ready for detailed review.

@mwleeds, @matthiasclasen, would you mind taking a look at this? (Matthias, I don’t know if you’re taking over all code review while Alex is on holiday, but I thought I should ask anyway. :-) )

Collaborator

pwithnall commented Jul 7, 2017

I’ve just force-pushed an updated version of the branch, rebased on master and with quite a few fixes squashed in. It should now have all the necessary #ifdefs. There’s one niggle about conditional enum parsing for glib-mkenums which I need to solve at some point, but this is ready for detailed review.

@mwleeds, @matthiasclasen, would you mind taking a look at this? (Matthias, I don’t know if you’re taking over all code review while Alex is on holiday, but I thought I should ask anyway. :-) )

@pwithnall pwithnall changed the title from (WIP) Peer to peer and unsigned summary file support to Peer to peer and unsigned summary file support Jul 7, 2017

@matthiasclasen

This comment has been minimized.

Show comment
Hide comment
@matthiasclasen

matthiasclasen Jul 7, 2017

Collaborator

i'm trying to keep obvious things flowing. This one may not be obvious enough for me, but I can give it a look

Collaborator

matthiasclasen commented Jul 7, 2017

i'm trying to keep obvious things flowing. This one may not be obvious enough for me, but I can give it a look

@mwleeds

I found a few minor things to point out but after that this seems ready to merge.

Show outdated Hide outdated lib/flatpak-remote.c
#ifndef __GI_SCANNER__
g_object_class_install_property (object_class,
PROP_TYPE,
g_param_spec_enum ("type",

This comment has been minimized.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

This seems to be causing glib assertion failures, I think because of the conditional compilation.

(process:22011): GLib-GObject-CRITICAL **: g_param_spec_enum: assertion 'g_enum_get_value (enum_class, default_value) != NULL' failed

(process:22011): GLib-GObject-CRITICAL **: g_object_class_install_property: assertion 'G_IS_PARAM_SPEC (pspec)' failed

Those assertion failures also seem to be causing some unit tests to fail.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

This seems to be causing glib assertion failures, I think because of the conditional compilation.

(process:22011): GLib-GObject-CRITICAL **: g_param_spec_enum: assertion 'g_enum_get_value (enum_class, default_value) != NULL' failed

(process:22011): GLib-GObject-CRITICAL **: g_object_class_install_property: assertion 'G_IS_PARAM_SPEC (pspec)' failed

Those assertion failures also seem to be causing some unit tests to fail.

This comment has been minimized.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Urgh, this is fallout from me marking the FlatpakRemoteType enum members as to be skipped, to avoid them getting picked up by glib-mkenums even when P2P support is disabled (because it ignores the #ifdef FLATPAK_ENABLE_P2P around them). I think the fix here is to unconditionally skip them, and manually add a get_type() function for them (conditionally).

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Urgh, this is fallout from me marking the FlatpakRemoteType enum members as to be skipped, to avoid them getting picked up by glib-mkenums even when P2P support is disabled (because it ignores the #ifdef FLATPAK_ENABLE_P2P around them). I think the fix here is to unconditionally skip them, and manually add a get_type() function for them (conditionally).

Show outdated Hide outdated common/flatpak-dir.c
@@ -5658,21 +6082,27 @@ flatpak_dir_update (FlatpakDir *self,
&gpg_verify_summary, error))
return FALSE;
if (!repo_get_remote_collection_id (self->repo, remote_name, &collection_id, NULL))
collection_id = NULL;

This comment has been minimized.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

Why don't we receive the error and give up as we do in most other places where repo_get_remote_collection_id is called?

@mwleeds

mwleeds Jul 18, 2017

Collaborator

Why don't we receive the error and give up as we do in most other places where repo_get_remote_collection_id is called?

This comment has been minimized.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Good catch. I was being relaxed about handling invalid configuration here, but there should be no harm in being strict.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Good catch. I was being relaxed about handling invalid configuration here, but there should be no harm in being strict.

Show outdated Hide outdated common/flatpak-dir.c
{
/* Don’t enable summary verification if a collection ID is set, as collection

This comment has been minimized.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

Don't we want to set "gpg-verify" to "true" when that option was specified, even if the collection_id is non-NULL?

@mwleeds

mwleeds Jul 18, 2017

Collaborator

Don't we want to set "gpg-verify" to "true" when that option was specified, even if the collection_id is non-NULL?

This comment has been minimized.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Eek, yes, good catch.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Eek, yes, good catch.

Show outdated Hide outdated common/flatpak-dir.c
download_size, installed_size,
metadata,
cancellable, error);
return flatpak_dir_parse_repo_metadata_for_ref (self, remote_name, ref,

This comment has been minimized.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

Maybe we should just get rid of flatpak_dir_fetch_ref_cache if it does the same thing as flatpak_dir_parse_repo_metadata_for_ref? Seems like technical debt.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

Maybe we should just get rid of flatpak_dir_fetch_ref_cache if it does the same thing as flatpak_dir_parse_repo_metadata_for_ref? Seems like technical debt.

This comment has been minimized.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Sure.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Sure.

Show outdated Hide outdated system-helper/flatpak-system-helper.c
{
if (!g_file_get_contents (arg_summary_sig_path, &summary_sig_data, &summary_sig_size, &error))
{
g_dbus_method_invocation_return_gerror (invocation, error);

This comment has been minimized.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

There's some extra whitespace here and on the following calls.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

There's some extra whitespace here and on the following calls.

This comment has been minimized.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Will fix in a separate commit.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Will fix in a separate commit.

Show outdated Hide outdated common/flatpak-dir.c
GError **error)
{
#ifdef FLATPAK_ENABLE_P2P
if (!ostree_repo_get_remote_option (repo, remote_name, "collection-id",

This comment has been minimized.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

We should probably check if collection_id_out is NULL before passing it to libOSTree since we're already checking that condition later in the function.

@mwleeds

mwleeds Jul 18, 2017

Collaborator

We should probably check if collection_id_out is NULL before passing it to libOSTree since we're already checking that condition later in the function.

This comment has been minimized.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Yup.

@pwithnall

pwithnall Aug 1, 2017

Collaborator

Yup.

@pwithnall

A few comments to address, then this should be good to land. Comments from @alexlarsson by e-mail:

On the plane I took a look at the p2p branch, in particular how it affects the non-p2p case. I found a few issues listed below, but otherwise I think this looks good to land now.

Show outdated Hide outdated common/flatpak-utils.c
continue;
/* Must match arch & branch */
if (!g_str_has_prefix (cur, ref_suffix))
continue;

This comment has been minimized.

@pwithnall

pwithnall Aug 3, 2017

Collaborator

Review comment from @alexlarsson on a plane: This reverts an existing fix (bad merge conflict resolution).

@pwithnall

pwithnall Aug 3, 2017

Collaborator

Review comment from @alexlarsson on a plane: This reverts an existing fix (bad merge conflict resolution).

This comment has been minimized.

@pwithnall

pwithnall Aug 3, 2017

Collaborator

Fixed.

@pwithnall

pwithnall Aug 3, 2017

Collaborator

Fixed.

@pwithnall

This comment has been minimized.

Show comment
Hide comment
@pwithnall

pwithnall Aug 3, 2017

Collaborator

Other things which we discussed which I need to do/look at:

  • Talk to Richard about "lib: List dynamic remotes in flatpak_installation_list_remotes()"
  • Add collection ID to the manifest file in "builder: Add collection ID support to the flatpak builder"
  • gnome-software has its own parser for flatpak{ref,repo} files; needs to get collection-id support
  • Eventually add migration support for remote configurations to add collection-id and set gpg-verify-summary=false; add the code for that now, but don’t put the config in repos yet
  • Double-check OCI code paths; explicitly state that OCI+collection-id is unsupported
  • See if there’s a nice way to run the entire test suite with collection-id set and unset; see test-repo-system.sh
Collaborator

pwithnall commented Aug 3, 2017

Other things which we discussed which I need to do/look at:

  • Talk to Richard about "lib: List dynamic remotes in flatpak_installation_list_remotes()"
  • Add collection ID to the manifest file in "builder: Add collection ID support to the flatpak builder"
  • gnome-software has its own parser for flatpak{ref,repo} files; needs to get collection-id support
  • Eventually add migration support for remote configurations to add collection-id and set gpg-verify-summary=false; add the code for that now, but don’t put the config in repos yet
  • Double-check OCI code paths; explicitly state that OCI+collection-id is unsupported
  • See if there’s a nice way to run the entire test suite with collection-id set and unset; see test-repo-system.sh
if (ostree_gpg_verify_result_count_valid (gpg_result) == 0)
return flatpak_fail (error, "GPG signatures found, but none are in trusted keyring");
}

This comment has been minimized.

@pwithnall

pwithnall Aug 3, 2017

Collaborator

Review comment from @alexlarsson on a plane: This seems to pass NULL for the remote, which is a magic option that means we're matching against the keyrings from all remotes. That does not seem correct.

@pwithnall

pwithnall Aug 3, 2017

Collaborator

Review comment from @alexlarsson on a plane: This seems to pass NULL for the remote, which is a magic option that means we're matching against the keyrings from all remotes. That does not seem correct.

This comment has been minimized.

@pwithnall

pwithnall Aug 3, 2017

Collaborator

Fixed.

@pwithnall

pwithnall Aug 3, 2017

Collaborator

Fixed.

This comment has been minimized.

@pwithnall

pwithnall Aug 3, 2017

Collaborator

(To clarify things: ostree_repo_verify_commit_ext() internally passes NULL for the remote name. We should be using ostree_repo_verify_commit_for_remote() instead, I think.)

@pwithnall

pwithnall Aug 3, 2017

Collaborator

(To clarify things: ostree_repo_verify_commit_ext() internally passes NULL for the remote name. We should be using ostree_repo_verify_commit_for_remote() instead, I think.)

@pwithnall

This comment has been minimized.

Show comment
Hide comment
@pwithnall

pwithnall Aug 3, 2017

Collaborator

Other things which we discussed which I need to do/look at:

  • Talk to Richard about "lib: List dynamic remotes in flatpak_installation_list_remotes()"

I’ve got a WIP branch here which changes gnome-software to ignore non-static remotes for now:
https://github.com/pwithnall/gnome-software/tree/lan-and-usb

  • Add collection ID to the manifest file in "builder: Add collection ID support to the flatpak builder"

Done. The builder: Add collection ID support to the flatpak builder commit has been updated to include this.

  • gnome-software has its own parser for flatpak{ref,repo} files; needs to get collection-id support

I can’t find such a parser. The code around line 2700 of gs-flatpak.c calls flatpak_installation_install_ref_file() with a flatpakref file which it loaded from disk (but didn’t parse) itself. Looks like gs_flatpak_app_install_source() gets called for flatpakrepo files, though, and it needs modifying.

WIP branch (which will be submitted to bugzilla.gnome.org eventually) here: https://github.com/pwithnall/gnome-software/tree/lan-and-usb

  • Eventually add migration support for remote configurations to add collection-id and set gpg-verify-summary=false; add the code for that now, but don’t put the config in repos yet

Added.

  • Double-check OCI code paths; explicitly state that OCI+collection-id is unsupported

Did you do this? I’m not sure of a good way/place to state that it’s unsupported.

  • See if there’s a nice way to run the entire test suite with collection-id set and unset; see test-repo-system.sh

Done; added test-repo-collections.sh and test-repo-collections-server-only.sh. This resulted in a fair few fixes, so you need to re-review commits from remote-add: Verify that GPG is enabled if collections are onwards.

Collaborator

pwithnall commented Aug 3, 2017

Other things which we discussed which I need to do/look at:

  • Talk to Richard about "lib: List dynamic remotes in flatpak_installation_list_remotes()"

I’ve got a WIP branch here which changes gnome-software to ignore non-static remotes for now:
https://github.com/pwithnall/gnome-software/tree/lan-and-usb

  • Add collection ID to the manifest file in "builder: Add collection ID support to the flatpak builder"

Done. The builder: Add collection ID support to the flatpak builder commit has been updated to include this.

  • gnome-software has its own parser for flatpak{ref,repo} files; needs to get collection-id support

I can’t find such a parser. The code around line 2700 of gs-flatpak.c calls flatpak_installation_install_ref_file() with a flatpakref file which it loaded from disk (but didn’t parse) itself. Looks like gs_flatpak_app_install_source() gets called for flatpakrepo files, though, and it needs modifying.

WIP branch (which will be submitted to bugzilla.gnome.org eventually) here: https://github.com/pwithnall/gnome-software/tree/lan-and-usb

  • Eventually add migration support for remote configurations to add collection-id and set gpg-verify-summary=false; add the code for that now, but don’t put the config in repos yet

Added.

  • Double-check OCI code paths; explicitly state that OCI+collection-id is unsupported

Did you do this? I’m not sure of a good way/place to state that it’s unsupported.

  • See if there’s a nice way to run the entire test suite with collection-id set and unset; see test-repo-system.sh

Done; added test-repo-collections.sh and test-repo-collections-server-only.sh. This resulted in a fair few fixes, so you need to re-review commits from remote-add: Verify that GPG is enabled if collections are onwards.

Show outdated Hide outdated common/flatpak-dir.c
g_variant_new_variant (g_variant_new_boolean (TRUE)));
g_variant_builder_add (&pull_builder, "{s@v}", "disable-static-deltas",
g_variant_new_variant (g_variant_new_boolean (TRUE)));
}

This comment has been minimized.

@alexlarsson

alexlarsson Aug 18, 2017

Member

This line is duplicated

@alexlarsson

alexlarsson Aug 18, 2017

Member

This line is duplicated

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Not entirely: the two copies add to different GVariantBuilder instances. I couldn’t think of a nice way to refactor that to not duplicate it.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Not entirely: the two copies add to different GVariantBuilder instances. I couldn’t think of a nice way to refactor that to not duplicate it.

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

(Fixed in 4bf896e.)

@pwithnall

pwithnall Aug 18, 2017

Collaborator

(Fixed in 4bf896e.)

Show outdated Hide outdated common/flatpak-dir.c
if (dirs_to_pull)
if (collection_id != NULL)

This comment has been minimized.

@alexlarsson

alexlarsson Aug 18, 2017

Member

It would be nice if a lot of the variant builder code here with the regular pull. For instance all the options are the same.

@alexlarsson

alexlarsson Aug 18, 2017

Member

It would be nice if a lot of the variant builder code here with the regular pull. For instance all the options are the same.

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Maybe there is some subsequent refactoring to do here. I left it as-is because it was easier, and also makes the review a bit easier than a more complex refactor.

Something to address once this branch is merged?

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Maybe there is some subsequent refactoring to do here. I left it as-is because it was easier, and also makes the review a bit easier than a more complex refactor.

Something to address once this branch is merged?

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

I’ve got a local commit to refactor this; I’ll create a PR once this is merged (since github doesn’t support PRs which depend on other ones).

@pwithnall

pwithnall Aug 18, 2017

Collaborator

I’ve got a local commit to refactor this; I’ll create a PR once this is merged (since github doesn’t support PRs which depend on other ones).

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Actually, I just pushed it to this PR: 4bf896e

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Actually, I just pushed it to this PR: 4bf896e

Show outdated Hide outdated common/flatpak-dir.c
g_variant_builder_add (&find_builder, "{s@v}", "update-frequency",
g_variant_new_variant (g_variant_new_uint32 (update_freq)));
g_variant_builder_add (&pull_builder, "{s@v}", "update-frequency",
g_variant_new_variant (g_variant_new_uint32 (update_freq)));

This comment has been minimized.

@alexlarsson

alexlarsson Aug 18, 2017

Member

This line is duplicated

@alexlarsson

alexlarsson Aug 18, 2017

Member

This line is duplicated

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Not entirely: the two copies add to different GVariantBuilder instances. I couldn’t think of a nice way to refactor that to not duplicate it.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Not entirely: the two copies add to different GVariantBuilder instances. I couldn’t think of a nice way to refactor that to not duplicate it.

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

(Fixed in 4bf896e.)

@pwithnall

pwithnall Aug 18, 2017

Collaborator

(Fixed in 4bf896e.)

Show outdated Hide outdated lib/flatpak-remote.h
@@ -25,6 +25,15 @@
#ifndef __FLATPAK_REMOTE_H__
#define __FLATPAK_REMOTE_H__
#ifdef FLATPAK_ENABLE_P2P
typedef enum

This comment has been minimized.

@alexlarsson

alexlarsson Aug 18, 2017

Member

Please don't make the libflatpak public headers ifdef:ed.
It should be completely safe to always have these available, as they are purely declarative and there is no way to create such remotes anyway. That means even if they have to change later (which i doubt) that doesn't really matter.

@alexlarsson

alexlarsson Aug 18, 2017

Member

Please don't make the libflatpak public headers ifdef:ed.
It should be completely safe to always have these available, as they are purely declarative and there is no way to create such remotes anyway. That means even if they have to change later (which i doubt) that doesn't really matter.

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

OK. I added the #ifdefs to avoid breaking public API later if they do have to change — but if you’re happy with that, I’m happy with removing that #ifdef mess. Do you want me to do that as an update to this PR, or as a follow-up PR?

@pwithnall

pwithnall Aug 18, 2017

Collaborator

OK. I added the #ifdefs to avoid breaking public API later if they do have to change — but if you’re happy with that, I’m happy with removing that #ifdef mess. Do you want me to do that as an update to this PR, or as a follow-up PR?

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Fixed and needing review in eddd208.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Fixed and needing review in eddd208.

Show outdated Hide outdated lib/flatpak-ref.h
@@ -71,4 +71,8 @@ FLATPAK_EXTERN char * flatpak_ref_format_ref (FlatpakRef *self);
FLATPAK_EXTERN FlatpakRef * flatpak_ref_parse (const char *ref,
GError **error);
#ifdef FLATPAK_ENABLE_P2P
FLATPAK_EXTERN const char * flatpak_ref_get_collection_id (FlatpakRef *self);
#endif /* FLATPAK_ENABLE_P2P */

This comment has been minimized.

@alexlarsson

alexlarsson Aug 18, 2017

Member

Same here. Lets make this non-optional. Its just never going to return anything but NULL anyway.

@alexlarsson

alexlarsson Aug 18, 2017

Member

Same here. Lets make this non-optional. Its just never going to return anything but NULL anyway.

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Sure. Same question about whether you want that done in this PR or in a follow-up.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Sure. Same question about whether you want that done in this PR or in a follow-up.

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Also, if we’re making the getter function unconditionally public, I assume you also want the FlatpakRef::collection-id property to be unconditionally public?

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Also, if we’re making the getter function unconditionally public, I assume you also want the FlatpakRef::collection-id property to be unconditionally public?

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Fixed and needing review in eddd208, including making FlatpakRef::collection-id public.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Fixed and needing review in eddd208, including making FlatpakRef::collection-id public.

installation ? installation : "",
cancellable,
error))
return FALSE;

This comment has been minimized.

@alexlarsson

alexlarsson Aug 18, 2017

Member

I wonder if this is the right approach. In the previous system the summary cache was per-user, but now you push the cache to the global repo. This means you might get e.g. authentication dialogs on read-only operations.

I think we need to change this, making the ostree-metadata cache a per-user thing.

@alexlarsson

alexlarsson Aug 18, 2017

Member

I wonder if this is the right approach. In the previous system the summary cache was per-user, but now you push the cache to the global repo. This means you might get e.g. authentication dialogs on read-only operations.

I think we need to change this, making the ostree-metadata cache a per-user thing.

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Right. Is there a per-user repo already which we could pull into? I’m not 100% sure about how all the system-helper bits interact and where they all live.

Is this something which could be deferred to a follow-up task rather than blocking this PR?

@pwithnall

pwithnall Aug 18, 2017

Collaborator

Right. Is there a per-user repo already which we could pull into? I’m not 100% sure about how all the system-helper bits interact and where they all live.

Is this something which could be deferred to a follow-up task rather than blocking this PR?

This comment has been minimized.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

We’re deferring this to a follow-up task.

@pwithnall

pwithnall Aug 18, 2017

Collaborator

We’re deferring this to a follow-up task.

pwithnall added some commits May 16, 2017

lib: List dynamic remotes in flatpak_installation_list_remotes()
Also expose a new flatpak_remote_get_remote_type() API so that users can
query what type of remote something is — whether it’s a USB or LAN
remote, or something statically configured.

Make this all conditional on compiling with --enable-p2p.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
app: Add support for collection IDs to built-in flatpak commands
This sets the collection ID on remote configs and in commit metadata
when building flatpaks.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
builder: Add collection ID support to the flatpak builder
Pass a --collection-id argument through to `flatpak build-export`.

Also add a ‘collection-id’ property to manifest files, which can be used
to set the collection ID on an exported repo (when using --repo) without
having to provide a command line option.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
build: Add an --enable-p2p configure option for the peer to peer feature
A series of following commits will introduce a peer to peer feature for
pulling apps and runtimes from LAN peers and USB sticks without needing
an internet connection. This requires experimental API in libostree
(which needs to have been configured with --enable-experimental-api), so
needs to be hidden behind a configure option in flatpak too. It’s called
--enable-p2p, and bumps our libostree dependency to 2017.8 with
experimental API required too.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common: Support collections in check_for_updates() and forward the re…
…sults

Search for updates on peer to peer sources as well as the internet in
check_for_updates(), and pass the resulting OstreeRepoFinderResult array
to the pull() calls, so a consistent set of checksums are pulled.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
lib/ref: Add collection ID support to FlatpakRef
This adds a new collection-id property which is only enabled if
FLATPAK_ENABLE_P2P is defined. The internal machinery for handling it is
always enabled, to reduce the amount of #ifdef spam.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
tests: Add a test for unsigned summary support
This relies on peer to peer support also being enabled; the test is
skipped otherwise.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

pwithnall added some commits Jun 30, 2017

common/dir: Support new experimental libostree API for finding remotes
This adds support in flatpak-dir.c for using the new libostree API for
finding remotes dynamically for a given set of refs, if flatpak is
configured with --enable-p2p.

The new code paths are only taken if the repository is configured with
a collection ID set.

These changes by themselves aren’t sufficient for full P2P support, as
all the infrastructure for downloading summary files and finding refs
needs to be modified in the following commits.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common: Support collection IDs in flatpak{ref,repo} and bundle files
These are loaded from the ref/repo/bundle metadata and added to the new
remote configuration.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/utils: Add flatpak_repo_set_collection_id() helper method
It will be used by builtins-repo-update in a following commit to allow
updating the collection ID for an upstream repo.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common: Support unsigned summary files and separate repo metadata
In order to eliminate some race conditions around updating the
summary{,.sig} file on the server, and to decouple signing the summary
from signing commits, and to support peer to peer mirrors of content
from multiple upstream collections: add support for unsigned summary
files.

This relaxes the requirement for gpg-verify-summary=true iff
collection-id is set in a remote’s local configuration. It depends on
some pending libostree changes to verify the ref for each commit using
the commit’s signed metadata. See
ostreedev/ostree#983.

Metadata storage has moved from the summary file to a new
ostree-metadata well-known branch on each repository, since this can be
signed for each update and for each collection separately. If the
collection-id is set in a remote’s local configuration, flatpak will
retrieve all repository metadata from this branch rather than from the
summary file. If collection-id is unset, it will ignore this branch and
continue to use the summary file, which will continue to be updated (and
externally signed as summary.sig) for backwards compatibility.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
app/repo: Load repository metadata from ostree-metadata ref if possible
Newer repositories will store metadata there, rather than in the summary
file (although the summary file will still be updated where possible for
backwards compatibility).

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Factor out a helper function to get a remote’s collection ID
We need to consistently handle the case where the collection ID is set
to the empty string (and treat it the same as if it were unset). Best
done in a helper function.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Remove an unused method
Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Add some FIXMEs for future improvements to collections
Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/utils: Add collection ID support for appstream/* branches
Also add collection and ref binding metadata to the generated appstream
commits, so they can be verified when using unsigned summary files.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common: Support collection IDs for related refs and extensions
Add support for collection IDs to the code which finds and pulls
related refs and other extensions.

Currently, related refs must have the same collection ID as the parent
ref — this is the most likely scenario anyway. In future, it should be
possible to extend the code to support pulling related refs from other
collections.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Verify commit bindings when pulling from an untrusted repo
When pulling from a local, untrusted repo (i.e. one which the user
downloaded into, and we want to pull into the trusted system repo),
verify the collection ID and ref bindings in the commit metadata for
each commit.

This is something which is normally done by libostree, but since we’re
rewriting the commit manually, we’re bypassing that part of the pull()
code path.

This is an inlined version of the check from verify_bindings() in
libostree.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Fix error handling for flatpak_dir_lookup_repo_metadata()
It can return FALSE with an error set, or FALSE without one set, which
indicates the key was not found.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Handle NULL out parameter correctly when getting config
ostree_repo_get_remote_option() requires the out parameter to be
non-NULL.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Propagate errors from invalid configurations
Rather than silently ignoring them. Note that invalid configurations
are distinct from missing configuration keys.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Drop an unnecessary wrapper function
This introduces no functional changes.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
system-helper: Fix incorrect whitespace
This introduces no functional changes.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
lib/remote: Add enum GType for FlatpakRemoteType
This will only be built when configured with P2P support. We can’t use
glib-mkenums here, as it doesn’t know about the #ifdef
FLATPAK_ENABLE_P2P which surrounds the enum definition. By manually
writing the get_type() function, we can surround it by #ifdefs as
appropriate.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Support updating collection-id from remote configuration
To allow staged deployment of collection-ID-based repositories,
introduce the code to update a local repository configuration to add a
collection ID to it, based on updated metadata from the remote (as is
currently supported for other configuration keys).

As a security measure, this only allows updating the collection ID from
an empty to a non-empty value. We do not allow collection IDs to be
renamed (or a malicious repository owner could bypass the user’s manual
verification of the collection ID by changing it after the user has
configured an unrelated remote).

The idea is that most repositories should remain without collection IDs
for now, and use this mechanism to set their collection IDs in future,
once the functionality is more stable.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/utils: Allow collection-id to be updated from repo config
In order to provide a transition path for repositories to add collection
IDs to themselves and propagate those collection IDs to clients’ remote
configurations, add another repo config key which controls whether the
repository’s collection ID is published. If xa.collection-id is set in
the repo’s published metadata, the client will update its configuration
to the given ID — but only if no ID is set already. This is a one-time
transition to prevent malicious repositories from remotely changing the
user’s configuration to associate their remote with a well-known
collection ID they don’t own.

Add a test for this.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
lib/remote: Add getter/setter for collection IDs
This isn’t really used internally, but will be used by gnome-software
for when it configures new flatpak remotes.

This is new public API, but is only declared if compiling with
--enable-p2p.

Includes some basic smoketests.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
tests: Use AM_TESTS_ENVIRONMENT rather than TESTS_ENVIRONMENT
The latter is reserved for the user to set in their environment. The
former is what the makefile is supposed to set.

See
https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
remote-add: Verify that GPG is enabled if collections are
Emit an error message if a collection ID is specified but GPG is not
enabled, since pulling using collection IDs requires GPG.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
repo-update: Disallow changing collection IDs
Emit an error message if the user tries to change the collection ID of
an existing repository between two non-empty values. Allow them to set a
collection ID where one was not set before. Changing the collection ID
once it’s already been set will break updates for all clients who have
previously pulled from the repository.

If a developer really wants to change the collection ID for a
repository, they’re going to have to recreate the repository from
scratch.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common: Clarify some error and debug messages in flatpak-dir.c
Signed-off-by: Philip Withnall <withnall@endlessm.com>
common: Prevent pulling ostree-metadata unless gpg-verify is true
Add a sanity check and error message which prevents pulling the
ostree-metadata ref (the repository metadata) unless GPG verification is
enabled, as it needs to be signed to be trusted.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
tests: Add support for collection IDs to test-repo.sh
This adds variable support for collection IDs: they can either be
enabled on the server, on the server and client, or not at all. If
enabled on the server, apps and runtimes are built with collection IDs
and the repository has one set. If enabled on the client, the remote
config is added to the local repository with a collection ID and GPG
verification enabled. They are controlled with
USE_COLLECTIONS_IN_{SERVER,CLIENT}={yes,no}.

These variables are used in the new wrapper tests,
test-repo-collections.sh and test-repo-collections-server-only.sh.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
lib/remote: Handle clearing a collection ID in FlatpakRemote
Squash "" to NULL.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
tests: Drop use of GMainLoop in testlibrary
Use g_main_context_iteration() manually in a loop instead; this makes
the termination conditions more obvious. This does not change the
behaviour of the test.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
tests: Print spawned program argv in testlibrary
This makes debugging failures a little easier.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
tests: Increase timeout values in testlibrary
1s is apparently not enough to install the test application on my
machine.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common: Ensure ostree-metadata pulls are cached
When the summary file is updated, it’s kept in a local cache, so that
parts of flatpak can refresh it at will without network impact. We need
the same for the ostree-metadata ref, which stores the repository’s
metadata when collection IDs and P2P are used.

Implement that by comparing the checksum of the ostree-metadata ref from
the summary file and from the local repository. If they differ, it’s
almost certainly going to be because the summary file is advertising a
more up-to-date ostree-metadata ref, which we should pull. If they don’t
differ, there’s no need to try and update the ref. Therefore, this
chains off the caching of the summary file.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
common/dir: Refactor handling for pull options
Factor out the pull options which are common to the collection-based and
non-collection-based code paths. This should make the code a little
easier to read.

This introduces no functional changes.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
lib: Make P2P API unconditionally public
Rather than a lot of #ifdef mess in the public headers, Alex would
prefer that the P2P API is made unconditionally public. This assumes
that they are unlikely to change in future. If they do, we just make
them return NULL or break API and drop them.

Signed-off-by: Philip Withnall <withnall@endlessm.com>
@pwithnall

This comment has been minimized.

Show comment
Hide comment
@pwithnall

pwithnall Aug 18, 2017

Collaborator

I’ve just autosquashed in the final fixups. This now has all the review fixes squashed in, and is ready to merge.

Collaborator

pwithnall commented Aug 18, 2017

I’ve just autosquashed in the final fixups. This now has all the review fixes squashed in, and is ready to merge.

@alexlarsson alexlarsson merged commit 6321bc9 into flatpak:master Aug 18, 2017

@pwithnall pwithnall deleted the pwithnall:lan-and-usb branch Aug 18, 2017

@pwithnall

This comment has been minimized.

Show comment
Hide comment
@pwithnall

pwithnall Aug 18, 2017

Collaborator

I’ve filed three follow-up tasks for issues discussed above during the review:

Collaborator

pwithnall commented Aug 18, 2017

I’ve filed three follow-up tasks for issues discussed above during the review:

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