[merged] Fetcher cleanup #636

cgwalters · 2016-12-23T01:37:53Z

No description provided.

This is in preparation for the libcurl port. We're basically making public what we had internally. The next step here is to create `ostree-fetcher-util.[ch]` that only operates in terms of this lower level API. Also drop the `_mirrored` from the function name since it's the default now.

Conceptually these now lay on top of the core API, and don't reference libsoup. This is preparation for libcurl porting, but it's also just generally better.

The previous commit introduced a single low level API - however, we can do things in a more optimal way for the curl backend if we drop the "streaming API" variant. Currently, we only use it to synchronously splice into a memory buffer...which is pretty silly when we could just do that in the backend. The only tweak here is that we have an "add NUL character" flag that is (possibly) needed when fetching into a membuf. The code here ends up being better I think, since we avoid the double return value for the `_finish()` invocation, and now most of the fetcher code (in the soup case) writes to a `GOutputStream` consistently. This will again make things easier for a curl backend.

I'm introducing a new binary in a later patch, and it makes sense to move more things to be common into the common section. Also I noticed we were missing an inclusion of common `$(AM_LDFLAGS)`, though AFAIK this doesn't break anything right now.

Working on the libcurl backend, I hit the issue that the trivial-httpd program depends on libsoup. I briefly considered having two versions, but libcurl is client only, and moreover trivial-httpd is no longer trivial - it has various features which are used by the test suite extensively. Hence, what we'll do is build it as a separate binary which links to libsoup, and use it during the tests. We *also* currently still provide `ostree trivial-httpd` since some things use it like `rpm-ostree-toolbox` and the Cockpit tests. After those are ported to use some other webserver, I plan to add a build-time option to drop it.

For the pending libcurl port, the backend is a bit more sensitive to the main context setup. The delta superblock fetch here is a synchronous request in the section that's supposed to be async. Now, libsoup definitely supports mixing sync and async requests, but it wasn't hard to help the libcurl port here by making this one async. Now fetchers are either sync or async.

cgwalters · 2017-01-03T18:54:58Z

OK, this one should be good to review and land I think.

I'm trying to add a libcurl backend to ostree, and I'd like people to stop using trivial-httpd; it was a mistake to have it be command line API. Related: ostreedev/ostree#636 (Note I didn't test this commit)

jlebon

Good stuff, I like the fetcher simplifications. Just a small nit.

jlebon · 2017-01-04T15:48:46Z

src/libostree/ostree-fetcher.c

+gboolean
+_ostree_fetcher_request_to_membuf_finish (OstreeFetcher *self,
+                                          GAsyncResult  *result,
+                                          GBytes       **out_filename,


out_bytes ?

jlebon · 2017-01-04T15:49:08Z

src/libostree/ostree-fetcher.h

+
+gboolean _ostree_fetcher_request_to_membuf_finish (OstreeFetcher *self,
+                                                   GAsyncResult  *result,
+                                                   GBytes       **out_filename,


out_bytes ?

cgwalters · 2017-01-04T16:13:41Z

☑️

jlebon · 2017-01-04T16:14:34Z

@rh-atomic-bot r+ 78609ca

rh-atomic-bot · 2017-01-04T16:14:42Z

⌛ Testing commit 78609ca with merge 062cc7e...

This is in preparation for the libcurl port. We're basically making public what we had internally. The next step here is to create `ostree-fetcher-util.[ch]` that only operates in terms of this lower level API. Also drop the `_mirrored` from the function name since it's the default now. Closes: #636 Approved by: jlebon

Conceptually these now lay on top of the core API, and don't reference libsoup. This is preparation for libcurl porting, but it's also just generally better. Closes: #636 Approved by: jlebon

The previous commit introduced a single low level API - however, we can do things in a more optimal way for the curl backend if we drop the "streaming API" variant. Currently, we only use it to synchronously splice into a memory buffer...which is pretty silly when we could just do that in the backend. The only tweak here is that we have an "add NUL character" flag that is (possibly) needed when fetching into a membuf. The code here ends up being better I think, since we avoid the double return value for the `_finish()` invocation, and now most of the fetcher code (in the soup case) writes to a `GOutputStream` consistently. This will again make things easier for a curl backend. Closes: #636 Approved by: jlebon

I'm introducing a new binary in a later patch, and it makes sense to move more things to be common into the common section. Also I noticed we were missing an inclusion of common `$(AM_LDFLAGS)`, though AFAIK this doesn't break anything right now. Closes: #636 Approved by: jlebon

Working on the libcurl backend, I hit the issue that the trivial-httpd program depends on libsoup. I briefly considered having two versions, but libcurl is client only, and moreover trivial-httpd is no longer trivial - it has various features which are used by the test suite extensively. Hence, what we'll do is build it as a separate binary which links to libsoup, and use it during the tests. We *also* currently still provide `ostree trivial-httpd` since some things use it like `rpm-ostree-toolbox` and the Cockpit tests. After those are ported to use some other webserver, I plan to add a build-time option to drop it. Closes: #636 Approved by: jlebon

For the pending libcurl port, the backend is a bit more sensitive to the main context setup. The delta superblock fetch here is a synchronous request in the section that's supposed to be async. Now, libsoup definitely supports mixing sync and async requests, but it wasn't hard to help the libcurl port here by making this one async. Now fetchers are either sync or async. Closes: #636 Approved by: jlebon

rh-atomic-bot · 2017-01-04T16:16:52Z

💔 Test failed - status-atomicjenkins

jlebon · 2017-01-04T16:32:07Z

@rh-atomic-bot r+ ffcc250

rh-atomic-bot · 2017-01-04T16:32:16Z

⌛ Testing commit ffcc250 with merge d9f43cd...

Conceptually these now lay on top of the core API, and don't reference libsoup. This is preparation for libcurl porting, but it's also just generally better. Closes: #636 Approved by: jlebon

The previous commit introduced a single low level API - however, we can do things in a more optimal way for the curl backend if we drop the "streaming API" variant. Currently, we only use it to synchronously splice into a memory buffer...which is pretty silly when we could just do that in the backend. The only tweak here is that we have an "add NUL character" flag that is (possibly) needed when fetching into a membuf. The code here ends up being better I think, since we avoid the double return value for the `_finish()` invocation, and now most of the fetcher code (in the soup case) writes to a `GOutputStream` consistently. This will again make things easier for a curl backend. Closes: #636 Approved by: jlebon

I'm introducing a new binary in a later patch, and it makes sense to move more things to be common into the common section. Also I noticed we were missing an inclusion of common `$(AM_LDFLAGS)`, though AFAIK this doesn't break anything right now. Closes: #636 Approved by: jlebon

Working on the libcurl backend, I hit the issue that the trivial-httpd program depends on libsoup. I briefly considered having two versions, but libcurl is client only, and moreover trivial-httpd is no longer trivial - it has various features which are used by the test suite extensively. Hence, what we'll do is build it as a separate binary which links to libsoup, and use it during the tests. We *also* currently still provide `ostree trivial-httpd` since some things use it like `rpm-ostree-toolbox` and the Cockpit tests. After those are ported to use some other webserver, I plan to add a build-time option to drop it. Closes: #636 Approved by: jlebon

For the pending libcurl port, the backend is a bit more sensitive to the main context setup. The delta superblock fetch here is a synchronous request in the section that's supposed to be async. Now, libsoup definitely supports mixing sync and async requests, but it wasn't hard to help the libcurl port here by making this one async. Now fetchers are either sync or async. Closes: #636 Approved by: jlebon

rh-atomic-bot · 2017-01-04T16:44:27Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing d9f43cd to master...

trivial-httpd will be depreciated. Related: ostreedev/ostree#636

trivial-httpd will be depreciated. Related: ostreedev/ostree#636 Closes #5680 Closes #5682 Reviewed-by: Stef Walter <[email protected]>

cgwalters added 3 commits December 28, 2016 14:47

fetcher: Move high level functions into "fetcher-util"

c59b09e

Conceptually these now lay on top of the core API, and don't reference libsoup. This is preparation for libcurl porting, but it's also just generally better.

cgwalters force-pushed the fetcher-cleanup branch from ecd2b12 to 8dc0db3 Compare December 28, 2016 19:47

cgwalters added 8 commits December 29, 2016 16:05

build-sys: Minor makefile tweaks

6bde81d

I'm introducing a new binary in a later patch, and it makes sense to move more things to be common into the common section. Also I noticed we were missing an inclusion of common `$(AM_LDFLAGS)`, though AFAIK this doesn't break anything right now.

fixup! fetcher: Split lowlevel API into file/membuf variants

f31a541

fixup! pull: Rework delta superblock fetches to be async

3f78647

fixup! pull: Rework delta superblock fetches to be async

c42f552

fixup! Split trivial-httpd into separate binary

1287f9d

fixup! Split trivial-httpd into separate binary

0cdd8de

cgwalters mentioned this pull request Jan 4, 2017

tests: Stop using ostree trivial-httpd cockpit-project/cockpit#5680

Closed

jlebon self-requested a review January 4, 2017 15:37

jlebon requested changes Jan 4, 2017

View reviewed changes

fixup! fetcher: Split lowlevel API into file/membuf variants

78609ca

jlebon approved these changes Jan 4, 2017

View reviewed changes

fixup! fetcher: Split lowlevel API into file/membuf variants

ffcc250

rh-atomic-bot changed the title ~~Fetcher cleanup~~ [merged] Fetcher cleanup Jan 4, 2017

rh-atomic-bot closed this in af56004 Jan 4, 2017

petervo added a commit to petervo/cockpit that referenced this pull request Jan 5, 2017

test: Stop using ostree trivial-httpd

4ce3c6d

trivial-httpd will be depreciated. Related: ostreedev/ostree#636

petervo mentioned this pull request Jan 5, 2017

test: Stop using ostree trivial-httpd cockpit-project/cockpit#5682

Closed

stefwalter pushed a commit to cockpit-project/cockpit that referenced this pull request Jan 5, 2017

test: Stop using ostree trivial-httpd

0a6c048

trivial-httpd will be depreciated. Related: ostreedev/ostree#636 Closes #5680 Closes #5682 Reviewed-by: Stef Walter <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[merged] Fetcher cleanup #636

[merged] Fetcher cleanup #636

cgwalters commented Dec 23, 2016

cgwalters commented Jan 3, 2017

jlebon left a comment

jlebon Jan 4, 2017

jlebon Jan 4, 2017

cgwalters commented Jan 4, 2017

jlebon commented Jan 4, 2017

rh-atomic-bot commented Jan 4, 2017

rh-atomic-bot commented Jan 4, 2017

jlebon commented Jan 4, 2017

rh-atomic-bot commented Jan 4, 2017

rh-atomic-bot commented Jan 4, 2017

[merged] Fetcher cleanup #636

[merged] Fetcher cleanup #636

Conversation

cgwalters commented Dec 23, 2016

cgwalters commented Jan 3, 2017

jlebon left a comment

Choose a reason for hiding this comment

jlebon Jan 4, 2017

Choose a reason for hiding this comment

jlebon Jan 4, 2017

Choose a reason for hiding this comment

cgwalters commented Jan 4, 2017

jlebon commented Jan 4, 2017

rh-atomic-bot commented Jan 4, 2017

rh-atomic-bot commented Jan 4, 2017

jlebon commented Jan 4, 2017

rh-atomic-bot commented Jan 4, 2017

rh-atomic-bot commented Jan 4, 2017