create-usb: Add a create-usb command to complement OstreeRepoFinderMount #1182

pwithnall · 2017-09-18T13:13:13Z

This can be used to put OSTree repositories on USB sticks in a format
recognised by OstreeRepoFinderMount.

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

This depends on PR #1179 (and hence contains the same initial commits, because github can’t understand dependencies between PRs), and must be rebased and updated after PR #1179 is pushed.

pwithnall · 2017-09-18T13:39:33Z

Thinking about file systems, this either needs to check the file system of the destination repository (and check that it supports xattrs), or use a repository mode which doesn’t need xattrs (I think the only option in this case is archive-z2; we can’t use bare-user-only because we can’t lose the permissions since the repository isn’t a leaf checkout).

Colin, would it make sense for this tool to potentially eventually grow a --format option which wipes the USB stick before creating the repository? Or would you prefer to keep that separate?

pwithnall · 2017-09-18T14:39:14Z

Thinking about file systems, …

I’ve pushed fixups which check whether the destination file system supports xattrs. If it does, create-usb uses a bare-user repository; otherwise, it uses archive-z2. If the destination repository already exists, its existing mode is used (which might cause pull failures if the existing mode is inappropriate, but I think that’s reasonable behaviour).

rh-atomic-bot · 2017-09-18T17:21:41Z

☔ The latest upstream changes (presumably d0b0578) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters · 2017-09-19T18:10:52Z

Hm, need to think about this a bit more; big picture I'd been trying to slowly move away from the ostree command line as an end user thing (but obviously keep the non-ostree admin i.e. the repo management bits around).

This feels like sort of an in-between; not opposed or anything, just need to think about it a bit.

rh-atomic-bot · 2017-09-21T21:39:51Z

☔ The latest upstream changes (presumably 64b23fd) made this pull request unmergeable. Please resolve the merge conflicts.

Previously, collection–refs could only be pulled from a repository if it had a summary file (which listed them). There was no way to pull from a local repository which doesn’t have a summary file, and where the refs were stored as refs/remotes/$remote/$ref, with a config section linking that $remote to the queried collection ID. Fix that by explicitly supporting pull_data->remote_repo_local in fetch_ref_contents(). Signed-off-by: Philip Withnall <withnall@endlessm.com>

This can be used to put OSTree repositories on USB sticks in a format recognised by OstreeRepoFinderMount. Signed-off-by: Philip Withnall <withnall@endlessm.com>

pwithnall · 2017-09-22T11:53:32Z

☔️ The latest upstream changes (presumably 64b23fd) made this pull request unmergeable. Please resolve the merge conflicts.

Rebased (same for the other dependent PRs).

cgwalters · 2017-09-22T12:42:43Z

src/libostree/ostree-repo-pull.c

+                                  &refs, cancellable, error))
+        return FALSE;
+
+      ret_contents = g_strdup (g_hash_table_lookup (refs, ref->ref_name));


Can't we just ostree_repo_resolve_rev()?

Indeed, yes.

cgwalters · 2017-09-22T12:43:03Z

src/libostree/ostree-repo-pull.c

+                                             cancellable, error))
+        return FALSE;
+
+      ret_contents = g_strdup (g_hash_table_lookup (refs, ref));


(See below, this reminds me we need a collection equivalent of ostree_repo_resolve_rev().)

I’ll add a basic version of that in this PR, but it’ll basically be a splitting out of the code which is already in the PR. It can be optimised in a follow-up issue.

Yep, fine by me.

cgwalters · 2017-09-22T12:46:35Z

src/ostree/ot-builtin-create-usb.c

+  if (!glnx_fstatat_allow_noent (ostree_repo_get_dfd (dest_repo), "summary", &stbuf, 0, error))
+    return FALSE;
+  if (errno == ENOENT &&
+      !ostree_repo_regenerate_summary (dest_repo, NULL, cancellable, error))


Need to regenerate the summary always after doing a pull, right?

Not really; only when publishing the summary. Although the end result is equivalent.

cgwalters · 2017-09-22T12:48:53Z

src/ostree/ot-builtin-create-usb.c

+              if (ret < 0 && errno != EEXIST)
+                return glnx_throw_errno_prefix (error, "symlinkat(%s → %s)", symlink_path, relative_dest_repo_path);
+              else if (ret >= 0)
+                break;


There's a private _ostree_make_temporary_symlink_at() though it's harder to use private bits from the cmdline. It uses glnx_gen_temp_name() which is visible here.

Looks fairly similar to what I’ve got in ot-builtin-create-usb.c. Is there anything wrong with the code I’ve got already? I’d prefer to keep the template as %02u since that’s a bit more human-friendly than XXXXXX.

It's fine as is, just noting.

pwithnall · 2017-09-22T16:10:18Z

Summary of what @cgwalters and I discussed today:

Let’s keep the addition as ostree create-usb for now
I’ll talk to people within Endless about our future plans for whether to integrate creating a USB stick into the UI (which affects whether/when we should libraryise this)
I’ll also talk to @alexlarsson about whether this would make sense to add to the flatpak command line tool too
We need to ensure ostree create-usb remains behind --enable-experimental-api (this is currently the case in the PR) so we have the liberty to change/break/remove it at any point

This is a parallel for ostree_repo_resolve_rev_ext() which works on collection–refs. At the moment, the implementation is simple and uses ostree_repo_list_collection_refs(). In future, it could be rewritten to check the checksum directly rather than enumerating all potentially-relevant checksums. Signed-off-by: Philip Withnall <withnall@endlessm.com>

This is more efficient in the non-collection case; in the collection case, the implementation of ostree_repo_resolve_collection_ref() needs to be rewritten to improve efficiency. Signed-off-by: Philip Withnall <withnall@endlessm.com>

…inderMount

pwithnall · 2017-09-26T14:20:29Z

I’ve pushed a few updates.

pwithnall · 2017-09-26T14:25:23Z

Note that I’m still running through the tests on these updates, so don’t push anything yet! The code is good to review though.

cgwalters · 2017-09-26T14:23:14Z

src/libostree/ostree-repo-refs.c

+ostree_repo_resolve_collection_ref (OstreeRepo                    *self,
+                                    const OstreeCollectionRef     *ref,
+                                    gboolean                       allow_noent,
+                                    OstreeRepoResolveRevExtFlags   flags,


Maybe take this opportunity to add OSTREE_REPO_RESOLVE_REV_EXT_ALLOW_NOENT and drop the boolean?

(Although we already shipped ostree_repo_resolve_rev_ext() which has both...dunno)

cgwalters · 2017-09-26T14:29:15Z

Looks like the test case error matching will have to be adjusted:

++ sed -e 's/^/# /'
# error: Fetching checksum for ref ((empty), main): Invalid rev lots of html here  lots of html here  lots of html here  lots of
++ fatal 'File '\''err.txt'\'' doesn'\''t match fixed string list '\''error: Fetching refs/heads/main: Invalid rev lots of html here  lots of html here  lots of html here  lots of'\'''

cgwalters · 2017-09-26T14:29:36Z

LGTM aside from that!

pwithnall · 2017-09-26T15:15:18Z

Looks like the test case error matching will have to be adjusted:

Indeed. The rest of the tests pass. PR updated.

pwithnall · 2017-09-26T15:22:36Z

I’ll talk to people within Endless about our future plans for whether to integrate creating a USB stick into the UI (which affects whether/when we should libraryise this)

We do eventually plan to create a UI for creating a USB stick, but it’s a very long-term plan, and it’s not currently clear what shape the UI would be, or where it would be integrated.

I’ll also talk to @alexlarsson about whether this would make sense to add to the flatpak command line tool too

I briefly asked Alex and he seemed favourable to the idea of some kind of flatpak create-usb command, so that may happen at some point. I’m not going to work on that right now though.

So eventually we may end up libraryising the code in ostree create-usb, but I don’t think it makes sense to do that before we have a concrete plan for whacking it in a UI, for example.

cgwalters · 2017-09-26T15:23:55Z

@rh-atomic-bot r+ da2cf63

rh-atomic-bot · 2017-09-26T15:24:00Z

⌛ Testing commit da2cf63 with merge 9e36ff3...

rh-atomic-bot · 2017-09-26T15:35:20Z

💔 Test failed - status-atomicjenkins

pwithnall · 2017-09-26T17:21:47Z

@rh-atomic-bot, retest please

cgwalters · 2017-09-26T17:24:18Z

@rh-atomic-bot retry

cgwalters · 2017-09-26T17:25:26Z

Indeed seems to pass locally, and I can't see an obvious error (though unfortunately g-d-t-r makes it a pain...need to use --report-directory)

rh-atomic-bot · 2017-09-26T17:30:24Z

⌛ Testing commit da2cf63 with merge a871bf6...

Previously, collection–refs could only be pulled from a repository if it had a summary file (which listed them). There was no way to pull from a local repository which doesn’t have a summary file, and where the refs were stored as refs/remotes/$remote/$ref, with a config section linking that $remote to the queried collection ID. Fix that by explicitly supporting pull_data->remote_repo_local in fetch_ref_contents(). Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

This can be used to put OSTree repositories on USB sticks in a format recognised by OstreeRepoFinderMount. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

This is a parallel for ostree_repo_resolve_rev_ext() which works on collection–refs. At the moment, the implementation is simple and uses ostree_repo_list_collection_refs(). In future, it could be rewritten to check the checksum directly rather than enumerating all potentially-relevant checksums. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

This is more efficient in the non-collection case; in the collection case, the implementation of ostree_repo_resolve_collection_ref() needs to be rewritten to improve efficiency. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

rh-atomic-bot · 2017-09-26T17:42:56Z

💔 Test failed - status-atomicjenkins

cgwalters · 2017-09-26T18:05:53Z

Took a quick stab at a debugging aid in #1218

…inderMount

pwithnall · 2017-09-27T13:16:01Z

Looking at the log:

ok 4 adding ref to an existing usb
… (intermingled test output from parallel tests)
Imported 1 GPG key to remote "remote1"
FAIL: libostree/test-create-usb.sh.test (Child process exited with code 127)

I think the problem is this line in test-create-usb.sh:

${test_builddir}/repo-finder-mount finder-repo dest-mount1 org.example.Collection1 test-1 org.example.Collection1 test-2 &> out

Exit status 127 normally means ‘command not found’, which would fit with this.

Also from the log, we have test_builddir=/usr/libexec/installed-tests/libostree/tests, so it looks like I failed to make this particular test work when run installed.

Fix pushed.

pwithnall · 2017-09-27T14:30:25Z

All checks have passed

🎊😁🎆🎶

cgwalters · 2017-09-27T14:31:17Z

@rh-atomic-bot r+ d49a164

rh-atomic-bot · 2017-09-27T14:31:24Z

⌛ Testing commit d49a164 with merge a9b8441...

Previously, collection–refs could only be pulled from a repository if it had a summary file (which listed them). There was no way to pull from a local repository which doesn’t have a summary file, and where the refs were stored as refs/remotes/$remote/$ref, with a config section linking that $remote to the queried collection ID. Fix that by explicitly supporting pull_data->remote_repo_local in fetch_ref_contents(). Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

This can be used to put OSTree repositories on USB sticks in a format recognised by OstreeRepoFinderMount. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

This is a parallel for ostree_repo_resolve_rev_ext() which works on collection–refs. At the moment, the implementation is simple and uses ostree_repo_list_collection_refs(). In future, it could be rewritten to check the checksum directly rather than enumerating all potentially-relevant checksums. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

This is more efficient in the non-collection case; in the collection case, the implementation of ostree_repo_resolve_collection_ref() needs to be rewritten to improve efficiency. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

rh-atomic-bot · 2017-09-27T14:41:59Z

💔 Test failed - status-atomicjenkins

cgwalters · 2017-09-27T14:43:58Z

@rh-atomic-bot retry

rh-atomic-bot · 2017-09-27T14:44:04Z

⌛ Testing commit d49a164 with merge 2f9f722...

This can be used to put OSTree repositories on USB sticks in a format recognised by OstreeRepoFinderMount. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

This is a parallel for ostree_repo_resolve_rev_ext() which works on collection–refs. At the moment, the implementation is simple and uses ostree_repo_list_collection_refs(). In future, it could be rewritten to check the checksum directly rather than enumerating all potentially-relevant checksums. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

This is more efficient in the non-collection case; in the collection case, the implementation of ostree_repo_resolve_collection_ref() needs to be rewritten to improve efficiency. Signed-off-by: Philip Withnall <withnall@endlessm.com> Closes: #1182 Approved by: cgwalters

rh-atomic-bot · 2017-09-27T14:57:19Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 2f9f722 to master...

pwithnall self-assigned this Sep 18, 2017

pwithnall force-pushed the repo-finder-mount-tool branch from be45860 to d23a4e9 Compare September 19, 2017 09:52

This was referenced Sep 21, 2017

lib/repo-pull: Fix remote names in refspecs from non-mirror P2P pulls #1202

Closed

tests: Add a manual integration test for OstreeRepoFinderMount #1203

Closed

pwithnall force-pushed the repo-finder-mount-tool branch 2 times, most recently from a293fc6 to 230bb0d Compare September 22, 2017 11:09

create-usb: Add a create-usb command to complement OstreeRepoFinderMount

0c2e340

This can be used to put OSTree repositories on USB sticks in a format recognised by OstreeRepoFinderMount. Signed-off-by: Philip Withnall <withnall@endlessm.com>

pwithnall force-pushed the repo-finder-mount-tool branch from 230bb0d to 0c2e340 Compare September 22, 2017 11:52

cgwalters reviewed Sep 22, 2017

View reviewed changes

pwithnall added 3 commits September 26, 2017 15:15

fixup! create-usb: Add a create-usb command to complement OstreeRepoF…

ed1f73e

…inderMount

cgwalters reviewed Sep 26, 2017

View reviewed changes

fixup! src/pull: Support local pulls for collection–refs

da2cf63

fixup! create-usb: Add a create-usb command to complement OstreeRepoF…

d49a164

…inderMount

rh-atomic-bot closed this in f923c2e Sep 27, 2017

pwithnall deleted the repo-finder-mount-tool branch September 27, 2017 15:20

create-usb: Add a create-usb command to complement OstreeRepoFinderMount #1182

create-usb: Add a create-usb command to complement OstreeRepoFinderMount #1182

Conversation

pwithnall commented Sep 18, 2017 • edited Loading

pwithnall commented Sep 18, 2017

pwithnall commented Sep 18, 2017

rh-atomic-bot commented Sep 18, 2017

cgwalters commented Sep 19, 2017

rh-atomic-bot commented Sep 21, 2017

pwithnall commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwithnall commented Sep 22, 2017 • edited Loading

pwithnall commented Sep 26, 2017

pwithnall commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Sep 26, 2017

cgwalters commented Sep 26, 2017

pwithnall commented Sep 26, 2017

pwithnall commented Sep 26, 2017

cgwalters commented Sep 26, 2017

rh-atomic-bot commented Sep 26, 2017

rh-atomic-bot commented Sep 26, 2017

pwithnall commented Sep 26, 2017

cgwalters commented Sep 26, 2017

cgwalters commented Sep 26, 2017

rh-atomic-bot commented Sep 26, 2017

rh-atomic-bot commented Sep 26, 2017

cgwalters commented Sep 26, 2017

pwithnall commented Sep 27, 2017

pwithnall commented Sep 27, 2017

cgwalters commented Sep 27, 2017

rh-atomic-bot commented Sep 27, 2017

rh-atomic-bot commented Sep 27, 2017

cgwalters commented Sep 27, 2017

rh-atomic-bot commented Sep 27, 2017

rh-atomic-bot commented Sep 27, 2017

pwithnall commented Sep 18, 2017 •

edited

Loading

pwithnall commented Sep 22, 2017 •

edited

Loading