Make P2P API public (no longer experimental) #1596
Conversation
|
Can one of the admins verify this patch? |
|
@cgwalters could you do a release with this commit included in the near future? Sorry to mess up your normal schedule but I'm trying to get Flathub P2P enabled in time for Endless 3.4.1. Thanks! |
mwleeds
force-pushed
the
enable-experimental-api
branch
from
f52db34
to
2d3d161
Compare
|
Interestingly the CI failure here: isn't unique to this branch. You can reproduce it on the master branch using the configure flags used by the CI but with |
There was a problem hiding this comment.
I’ve looked over this from the point of view of whether the changes in the patch are correct, and they look correct. I haven’t really looked at it from the point of view of whether they’re complete (whether other changes need to be made to completely un-experimentalise the API and implementation).
I also haven’t reviewed each new API and command line tool to ensure they’re the right shape to be released (as stable API which has to be supported forever). I think they are the right shape, but I’m also pretty sure that we deferred some API discussions to “the point at which the experimental API is going to be made public”. So those reviews/discussions do need to happen.
| @@ -63,14 +63,12 @@ static OstreeCommand commands[] = { | |||
| { "export", OSTREE_BUILTIN_FLAG_NONE, | |||
| ostree_builtin_export, | |||
| "Stream COMMIT to stdout in tar format" }, | |||
| #ifdef OSTREE_ENABLE_EXPERIMENTAL_API | |||
| { "find-remotes", OSTREE_BUILTIN_FLAG_NONE, | |||
| ostree_builtin_find_remotes, | |||
| "Find remotes to serve the given refs" }, | |||
| { "create-usb", OSTREE_BUILTIN_FLAG_NONE, | |||
| ostree_builtin_create_usb, | |||
| "Copy the refs to a USB stick" }, | |||
There was a problem hiding this comment.
These two ostree subcommands (find-remotes and create-usb) need to be added to bash/ostree to get completion support.
|
☔ The latest upstream changes (presumably 9131d8a) made this pull request unmergeable. Please resolve the merge conflicts. |
mwleeds
force-pushed
the
enable-experimental-api
branch
2 times, most recently
from
9ec31b3
to
ac94b1d
Compare
|
Overall, it looks pretty good to me. Minor notes:
Actually that's it. |
That’s not entirely trivial to do, since they support different subsets of the options. Each of the options which is supported by both functions has the same semantics in both implementations. For example, Furthermore, an explicit |
|
@cgwalters: to be clear, are you happy with all of the API here being set in stone for ever more? |
mwleeds
force-pushed
the
enable-experimental-api
branch
from
ac94b1d
to
c859984
Compare
What would be the alternatives? Split it into a higher level library? Doesn't seem worth it. Something else? |
Tweak it now before making it public. I’m happy the API is the right shape and architecture. There might be naming niggles or minor changes you’d want to make around the function arguments or whatever, which might be annoying once stable. I was basically just double-checking that you’re happy with this and are not being pressured into getting it in quickly over getting it in correctly. |
|
For the record I also want to get it right and not just do it quickly. I've looked at most of this API to see if it seems future proof, and I'll look through it again before considering this ready to merge (still need to fix the build failures also of course). @pwithnall: you said there were discussions that were postponed to this point. Would they be on old PRs or somewhere else? |
I didn’t mean to imply otherwise!
I honestly can’t remember. There are a few |
mwleeds
force-pushed
the
enable-experimental-api
branch
4 times, most recently
from
8b0af03
to
041faf9
Compare
|
@mwleeds Can you split the prep commits to a separate PR? |
I'll get the CI happy with this branch before splitting it off; otherwise I'd have no way to know the prep commits have their intended effect. |
mwleeds
force-pushed
the
enable-experimental-api
branch
3 times, most recently
from
b55106c
to
da55f81
Compare
|
This is now blocked on https://gitlab.gnome.org/GNOME/libglnx/merge_requests/1 (and a libglnx update in ostree). That should fix the Travis build, and then the only remaining failure is of flatpak, but that's intentional and will have to be fixed on the flatpak side. |
|
Currently the flatpak context is blocking though; we could disable it, or land the prep fixes in flatpak first, then bump the CI context here to pull that newer flatpak version? |
Sure, we can do the latter |
mwleeds
force-pushed
the
enable-experimental-api
branch
2 times, most recently
from
5071557
to
dfbcfd4
Compare
|
☔ The latest upstream changes (presumably bf3525a) made this pull request unmergeable. Please resolve the merge conflicts. |
mwleeds
force-pushed
the
enable-experimental-api
branch
2 times, most recently
from
6ec1b66
to
02da0c5
Compare
|
bot, retest this please |
|
Alright, so I found all the FIXME/TODO/XXX comments in the affected files and looked at each. In my opinion none of them should be considered blockers to merging this PR, and only three seem like they have the potential to cause bugs (see attached if you want the details). "File","Line number","Comment","P2P related","Verdict" "src/libostree/ostree-core.c",2104,"/* TODO: make this public later; just wraps the previously public","no", "src/libostree/ostree-core.c",2131,"/* TODO */","no", "src/libostree/ostree-repo-commit.c",1172,"/* TODO: dedup this with commit_path_final() */","no", "src/libostree/ostree-repo-commit.c",3367,"* TODO: Do this lazily, since for e.g. bare-user-only repos","no", "src/libostree/ostree-repo-pull.c",1403,"/* TODO - make async */","no", "src/libostree/ostree-repo-pull.c",2507,"* them. TODO: Ideally this free space check would be above, but we'd have to","no", "src/libostree/ostree-repo-pull.c",3018,"/* TODO; port away from this; a bit hard since both libsoup and libcurl","no", "src/libostree/ostree-repo-pull.c",3847,"/* TODO reindent later */","no", "src/libostree/ostree-repo.c",3977,"/* TODO more consistency checks here */","no", "src/libostree/ostree-repo-commit.c",2097,"/* FIXME: Added OSTREE_SUPPRESS_SYNCFS since valgrind in el7 doesn't know","no", "src/libostree/ostree-repo-commit.c",4357,"/* FIXME - cleanup detached metadata if copy below fails */","no", "src/libostree/ostree-repo-finder-avahi.c",105,"/* FIXME: Submit these upstream */","yes","Non-critical" "src/libostree/ostree-repo-finder-avahi.c",110,"/* FIXME: Register this with IANA? https://tools.ietf.org/html/rfc6335#section-5.2 */","yes","Non-critical" "src/libostree/ostree-repo-finder-avahi.c",221,"/* FIXME: Need a better separator than `_`, since it’s not escaped in the input. */","yes","Non-critical but important" "src/libostree/ostree-repo-finder-avahi.c",1376,"/* FIXME: Make this a property */","yes","Non-critical" "src/libostree/ostree-repo-finder-mount.c",149,"/* FIXME: Need a better separator than `_`, since it’s not escaped in the input. */","yes","Non-critical but important" "src/libostree/ostree-repo-finder-override.c",96,"/* FIXME: Need a better separator than `_`, since it’s not escaped in the input. */","yes","Non-critical but important" "src/libostree/ostree-repo-finder.c",516,"/* FIXME: Check if this is really the ordering we want. For example, we","yes","Non-critical" "src/libostree/ostree-repo-pull.c",3246,"/* FIXME: Send the ETag from the cache with the request for summary.sig to","no", "src/libostree/ostree-repo-pull.c",3841,"/* FIXME: Do we want an analogue of this which supports collection IDs? */","yes","Non-critical" "src/libostree/ostree-repo-pull.c",5137,"/* FIXME: We currently do nothing with @progress. Comment out to assuage -Wunused-variable */","yes","Non-critical" "src/libostree/ostree-repo-pull.c",5186,"/* FIXME: In future, we also want to pull static delta superblocks in this","yes","Non-critical" "src/libostree/ostree-repo-pull.c",5202,"/* FIXME: All these downloads could be parallelised; that requires the","yes","Non-critical" "src/libostree/ostree-repo-pull.c",5365,"/* FIXME: Support remotes which have contenturl, mirrorlist, etc. */","yes","Non-critical" "src/libostree/ostree-repo-pull.c",5591,"* FIXME: It would be so much better if we could pass #OstreeRemote pointers","yes","Non-critical" "src/libostree/ostree-repo-pull.c",5756,"/* FIXME: Rework this code to pull in parallel where possible. At the moment","yes","Non-critical" "src/libostree/ostree-repo-pull.c",5828,"/* FIXME: We do nothing useful with @progress at the moment. */","yes","Non-critical" "src/libostree/ostree-repo-refs.c",117,"/* FIXME: Conflict detection needs to be extended to collection–refs","yes","Non-critical but important" "src/ostree/ot-builtin-prune.c",246,"/* FIXME: Do we also want to look at ostree_repo_list_collection_refs()? */","yes","Non-critical but important" "src/libostree/ostree-repo-pull.c",3784,"/* XXX: would be interesting to implement metalink as another source of","no", "src/libostree/ostree-repo.c",1697,"/* XXX Not sure it's worth failing if the group to remove","no", "src/libostree/ostree-repo.c",5062,"/* XXX This is a hackish way to indicate to use ALL remote-specific","no", "src/ostree/ot-remote-builtin-add.c",150,"* XXX Not sure this interacts well with if-not-exists since we don't","no", "src/ostree/ot-remote-builtin-add.c",168,"/* XXX If we ever add internationalization, use ngettext() here. */","no", |
|
Hm, not obvious to me what failed with the installed tests. |
|
I suppose this is a good opportunity for me to install Atomic so I can debug this. |
|
Let's just try it and see if it bounces. |
|
🙀 |
|
Ah. For excessive OCD, mind squashing the (effective) fixup commit into the main one? |
|
It's a pretty separate concern than the main commit, no? |
|
OK, but it's also a requirement of the next commit right? We don't bisect much today, but let's try to keep things working between commits too. |
|
Only if you're linking against an old glib |
This commit includes libglnx.h in ostree-autocleanups.h, so we get the g_autoptr backports wherever they're needed. Also, remove the "#include libglnx.h" lines elsewhere that are no longer needed.
Currently the API that allows P2P operations (e.g. pulling an ostree ref from a LAN or USB source) is hidden behind the configure flag --enable-experimental-api. This commit makes the API public and makes that flag essentially a no-op (leaving it in place in case we want to use it again in the future). The P2P API has been tested over the last several months and proven to work. This means that since we're no longer using the "experimental" feature flag, P2P builds of Flatpak will fail when using versions of OSTree from this commit onwards, until Flatpak is patched in the near future. If you want to build Flatpak < 0.11.8 with P2P enabled and link against OSTree 2018.6, you'll have to patch Flatpak. However, since Flatpak won't yet have a hard dependency on OSTree 2018.6, it needs a new way to determine if the P2P API in OSTree is available, so this commit adds a "p2p" feature flag. This way the feature set is more semantically correct than if we had continued to use the "experimental" feature flag. In addition to making the P2P API public, this commit makes the P2P unit tests run by default, removes the f27-experimental CI instance that's no longer needed, changes a few man pages to reflect the changes, and updates the bash completion script to accept the new commands and options.
mwleeds
force-pushed
the
enable-experimental-api
branch
from
02da0c5
to
22806ee
Compare
|
I changed the order of the commits |
Currently the API that allows P2P operations (e.g. pulling an ostree ref from a LAN or USB source) is hidden behind the configure flag --enable-experimental-api. This commit makes the API public and makes that flag essentially a no-op (leaving it in place in case we want to use it again in the future). The P2P API has been tested over the last several months and proven to work. This means that since we're no longer using the "experimental" feature flag, P2P builds of Flatpak will fail when using versions of OSTree from this commit onwards, until Flatpak is patched in the near future. If you want to build Flatpak < 0.11.8 with P2P enabled and link against OSTree 2018.6, you'll have to patch Flatpak. However, since Flatpak won't yet have a hard dependency on OSTree 2018.6, it needs a new way to determine if the P2P API in OSTree is available, so this commit adds a "p2p" feature flag. This way the feature set is more semantically correct than if we had continued to use the "experimental" feature flag. In addition to making the P2P API public, this commit makes the P2P unit tests run by default, removes the f27-experimental CI instance that's no longer needed, changes a few man pages to reflect the changes, and updates the bash completion script to accept the new commands and options. Closes: #1596 Approved by: cgwalters
|
☀️ Test successful - status-atomicjenkins |
|
can somebody provide more info how to use p2p via lan? |
|
Hi @vtolstov, Currently Endless OS is the only distribution with support for LAN ostree updates, and even there we have some bugs left to fix before it's a smooth experience (hopefully in the next few months). If you want to try to do LAN ostree updates on another Linux distribution you would need replacements for eos-update-server and eos-updater-avahi. However USB updates don't depend on those components, so for that you basically just need a repository with a collection ID set, as is being discussed here. See also |
|
@mwleeds Thanks! |
Currently the API that allows P2P operations (e.g. pulling an ostree ref
from a LAN or USB source) is hidden behind the configure flag
--enable-experimental-api. This commit makes the API public and makes
that flag essentially a no-op (leaving it in place in case we want to
use it again in the future). The P2P API has been tested over the last
several months and proven to work.
This means that since we're no longer using the "experimental" feature
flag, P2P builds of Flatpak will fail when using versions of OSTree from
this commit onwards, until Flatpak is patched in the near future. If you
want to build Flatpak < 0.11.8 with P2P enabled and link against OSTree
2018.6, you'll have to patch Flatpak. However, since Flatpak won't yet
have a hard dependency on OSTree 2018.6, it needs a new way to determine
if the P2P API in OSTree is available, so this commit adds a "p2p"
feature flag. This way the feature set is more semantically correct than
if we had continued to use the "experimental" feature flag.
In addition to making the P2P API public, this commit makes the P2P unit
tests run by default, changes a few man pages to reflect the changes,
updates the .gitignore, and updates the bash completion to account for
P2P related command line options.