Introduce `compose rojig` #1512

cgwalters · 2018-08-22T12:46:21Z

Starting to implement #1237

rh-atomic-bot · 2018-09-06T13:35:25Z

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

cgwalters · 2018-09-07T22:02:46Z

OK, this is updated again. The complexity in compose-builtin-tree.c is getting high, but I think it all makes sense.

The biggest pieces here are:

The relationship between workdir + repo gets inverted; the repo is just temporary. That is a big dance.
In pure rojig, we now need to do a query to find the previous rojig rpm.
And following on to that: In write-to-ostree mode, we would just load the commit object and get the version/inputhash. In pure rojig, we can't. So I added those to the self struct and gather them consistently from the data source (ostree repo or rojig rpm)

What I find myself wondering though is if we really need to support the case of "compose both to ostree and rojig" here. If we didn't support all 3 cases, it might end up cleaner to refactor to separate C files with shared helpers.

Anyone who wants to do both could just do rojig via commit2rojig afterwards, though that loses out on some ergonomics.

There's no tests yet but I have been testing this with some local silverblue 29 builds.

cgwalters · 2018-09-10T14:07:20Z

Test ✔️ added, lifting WIP.

jlebon · 2018-09-10T16:24:21Z

I haven't reviewed this yet -- can it wait until after the release?

cgwalters · 2018-09-10T16:31:00Z

can it wait until after the release?

Yep.

cgwalters · 2018-09-11T18:59:08Z

I think this is probably GTG as is, but we could consider making it more invasive.

jlebon · 2018-09-12T14:08:40Z

src/app/rpmostree-compose-builtin-tree.c

            return FALSE;
+          DnfContext *dnfctx = rpmostree_context_get_dnf (corectx);
+          if (!glnx_shutil_mkdir_p_at (self->workdir_dfd, "rojig-repos", 0755, cancellable, error))


Ouch, would be nice to enhance libdnf so we don't have to do this.

jlebon · 2018-09-12T14:09:55Z

src/app/rpmostree-compose-builtin-tree.c

+    {
+      if (!opt_ex_rojig_init)
+        {
+          g_autofree char *rojig_name = ror_treefile_get_rojig_name (self->treefile_rs);


Would be nice to factor all this out into a separate function.

jlebon · 2018-09-12T14:28:39Z

src/app/rpmostree-compose-builtin-tree.c

@@ -1698,7 +1770,8 @@ rpmostree_compose_builtin_tree (int             argc,
      return FALSE;
    }

-  if (!opt_repo)
+  const char *rojig_outputdir = opt_ex_jigdo_output_rpm ?: opt_ex_jigdo_output_set;
+  if (!opt_repo && !rojig_outputdir)


The various combinations of --repo, --ex-rojig-output-rpm, --ex-rojig-output-set, --cachedir, --workdir, --unified-core is hard to fully comprehend. I'm wondering if there are any ways we can split these flows apart now into distinct functions (or even files) across e.g. unified/non-unified, or ostree/rojig?

OK, and now I reread your older comment which mentions exactly this:

What I find myself wondering though is if we really need to support the case of "compose both to ostree and rojig" here. If we didn't support all 3 cases, it might end up cleaner to refactor to separate C files with shared helpers.

Yes, I think that makes sense to me. The major case I can imagine where you want both ostree and rojig is to reduce friction when transitioning from one to the other. But as you mentioned, one can still use commit2rojig.

OK so: split off a prep commit that drops the functionality, or do it in one go?

Splitting off would be nice, though if it's too entangled, doing it in one go is fine.

jlebon · 2018-09-12T14:34:09Z

src/app/rpmostree-compose-builtin-tree.c

+          self->previous_version = g_strdup (dnf_package_get_version (rojig_pkg));
+          self->previous_inputhash = g_strdup (rpmostree_context_get_rojig_inputhash (corectx));
+        }
+      else


Why not just drop --ex-rojig-init and enhance the core so we can discern the ENOENT case from other errors? Then it'd be fully symmetrical with the --repo case.

rh-atomic-bot · 2018-09-18T00:31:27Z

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

cgwalters · 2018-09-26T14:55:59Z

This conceptually depends on #1574 in that right now compose-builtin-rojig.c copy-pastes a whole of lot of compose-builtin-tree.c and that copy-paste would be significantly reduced if they shared a common Rust treefile parsing/processing base.

rh-atomic-bot · 2018-09-26T16:41:07Z

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

rh-atomic-bot · 2018-10-15T18:29:49Z

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

rh-atomic-bot · 2018-10-29T02:16:16Z

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

cgwalters · 2018-10-29T18:42:00Z

OK, lifting WIP on this.

rh-atomic-bot · 2018-10-30T15:04:56Z

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

src/app/rpmostree-compose-builtin-rojig.c

jlebon · 2018-10-30T19:00:46Z

src/app/rpmostree-compose-builtin-rojig.c

+  if (!glnx_shutil_mkdir_p_at (self->workdir_dfd, "rojig-repos", 0755, cancellable, error))
+    return FALSE;
+  { g_autofree char *repopath = g_strconcat ("rojig-repos/", rojig_output_repo_id, ".repo", NULL);
+    g_autofree char *repo_contents = g_strdup_printf ("[%s]\n"


Just thinking out loud here: this is just to find the previous rojig RPM, right? Hmm, I wonder if we could avoid this hack somehow by using dnf_sack_add_cmdline_package() instead. Another plus is that one doesn't have to createrepo_c the outdir first.

We'd have to scrape the dir still to find the latest...and at some point one needs to run createrepo_c to serve to clients.

jlebon · 2018-10-31T13:32:54Z

Looks sane to me, though the tests are hitting:

error[E0609]: no field `parsed` on type `&mut treefile::Treefile`
   --> src/treefile.rs:898:39
    |
898 |         if let &Some(ref rojig) = &tf.parsed.rojig {
    |                                       ^^^^^^

error[E0609]: no field `name` on type `&_`
   --> src/treefile.rs:899:32
    |
899 |             CString::new(rojig.name.as_str()).unwrap().into_raw()
    |                                ^^^^

error: aborting due to 2 previous errors

Sounds like it didn't test on top of the latest Rust PR? Anyway, Homu should rebase it, so let's give it a try.

@rh-atomic-bot r+ 147d201

rh-atomic-bot · 2018-10-31T13:32:59Z

⌛ Testing commit 147d201 with merge a10b5d5...

This currently requires a `--i-know-this-is-experimental` flag; I know it'd be a bit more consistent to have it under `ex`, but what feels weird about that is *most* of the `ex` commands people use are client side. This is where we want it to ultimately end up. We've landed a lot of prep patches, but I know there's still a notable amount of code duplication with `compose tree`. What's left is about ~700 lines but it's mostly not hard/complex code anymore. In the future, I'd like to extract more of the compose code to a `rust/src/compose.rs` or so, but I think this is sustainable fow now. My high level goal is to get this into coreos-assembler and stand up a Silverblue build that uses it. Closes: #1512 Approved by: jlebon

rh-atomic-bot · 2018-10-31T13:46:52Z

💔 Test failed - status-atomicjenkins

This currently requires a `--i-know-this-is-experimental` flag; I know it'd be a bit more consistent to have it under `ex`, but what feels weird about that is *most* of the `ex` commands people use are client side. This is where we want it to ultimately end up. We've landed a lot of prep patches, but I know there's still a notable amount of code duplication with `compose tree`. What's left is about ~700 lines but it's mostly not hard/complex code anymore. In the future, I'd like to extract more of the compose code to a `rust/src/compose.rs` or so, but I think this is sustainable fow now. My high level goal is to get this into coreos-assembler and stand up a Silverblue build that uses it.

cgwalters · 2018-10-31T17:39:42Z

Sounds like it didn't test on top of the latest Rust PR?

They actually did conflict, the parsed field is now used. Trivial fixup.

@rh-atomic-bot r=jlebon 4740e6e

rh-atomic-bot · 2018-10-31T17:39:49Z

⌛ Testing commit 4740e6e with merge 1e6b44e...

This currently requires a `--i-know-this-is-experimental` flag; I know it'd be a bit more consistent to have it under `ex`, but what feels weird about that is *most* of the `ex` commands people use are client side. This is where we want it to ultimately end up. We've landed a lot of prep patches, but I know there's still a notable amount of code duplication with `compose tree`. What's left is about ~700 lines but it's mostly not hard/complex code anymore. In the future, I'd like to extract more of the compose code to a `rust/src/compose.rs` or so, but I think this is sustainable fow now. My high level goal is to get this into coreos-assembler and stand up a Silverblue build that uses it. Closes: #1512 Approved by: jlebon

rh-atomic-bot · 2018-10-31T17:55:51Z

💔 Test failed - status-atomicjenkins

cgwalters · 2018-10-31T17:57:42Z

Now with actually pushing my fixup...
@rh-atomic-bot r=jlebon ada013a

rh-atomic-bot · 2018-10-31T17:57:47Z

⌛ Testing commit ada013a with merge 88ffdc0...

rh-atomic-bot · 2018-10-31T18:38:33Z

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

cgwalters mentioned this pull request Aug 22, 2018

rojig ♲📦: Prepare server-side design for rojig-only #1237

Open

cgwalters mentioned this pull request Sep 6, 2018

Rojig no repo prep #1528

Closed

cgwalters force-pushed the rojig-no-repo branch 2 times, most recently from f2a81cd to 23a1a64 Compare September 7, 2018 21:57

cgwalters force-pushed the rojig-no-repo branch from 23a1a64 to 77cc421 Compare September 10, 2018 14:05

cgwalters changed the title ~~WIP: support rojig-only mode~~ Support rojig-only mode Sep 10, 2018

cgwalters added for-next-release and removed for-next-release labels Sep 10, 2018

jlebon reviewed Sep 12, 2018

View reviewed changes

cgwalters changed the title ~~Support rojig-only mode~~ WIP: Support rojig-only mode Sep 13, 2018

cgwalters added the WIP label Sep 13, 2018

cgwalters force-pushed the rojig-no-repo branch from 77cc421 to bfc22b3 Compare September 21, 2018 19:46

cgwalters mentioned this pull request Oct 9, 2018

compose: Move "serialized treefile" into Rust ownership #1600

Closed

cgwalters force-pushed the rojig-no-repo branch from bfc22b3 to 9be5d26 Compare October 12, 2018 20:53

cgwalters force-pushed the rojig-no-repo branch from 9be5d26 to 4c1e575 Compare October 25, 2018 14:25

cgwalters force-pushed the rojig-no-repo branch 2 times, most recently from 39a137e to 1bd46b4 Compare October 29, 2018 17:17

cgwalters changed the title ~~WIP: Support rojig-only mode~~ Support rojig-only mode Oct 29, 2018

cgwalters removed the WIP label Oct 29, 2018

cgwalters changed the title ~~Support rojig-only mode~~ Introduce compose rojig Oct 29, 2018

cgwalters force-pushed the rojig-no-repo branch from 1bd46b4 to c31d7ae Compare October 30, 2018 15:18

jlebon reviewed Oct 30, 2018

View reviewed changes

jlebon added the reviewed/approved label Oct 31, 2018

cgwalters added 2 commits October 31, 2018 13:51

fixup! Add compose rojig command

4740e6e

cgwalters force-pushed the rojig-no-repo branch from 147d201 to 4740e6e Compare October 31, 2018 13:54

fixup! Add compose rojig command

ada013a

rh-atomic-bot closed this in 88ffdc0 Oct 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `compose rojig` #1512

Introduce `compose rojig` #1512

cgwalters commented Aug 22, 2018

rh-atomic-bot commented Sep 6, 2018

cgwalters commented Sep 7, 2018

cgwalters commented Sep 10, 2018

jlebon commented Sep 10, 2018

cgwalters commented Sep 10, 2018

cgwalters commented Sep 11, 2018

jlebon Sep 12, 2018

jlebon Sep 12, 2018

jlebon Sep 12, 2018

cgwalters Sep 12, 2018

jlebon Sep 12, 2018

jlebon Sep 12, 2018

rh-atomic-bot commented Sep 18, 2018

cgwalters commented Sep 26, 2018

rh-atomic-bot commented Sep 26, 2018

rh-atomic-bot commented Oct 15, 2018

rh-atomic-bot commented Oct 29, 2018

cgwalters commented Oct 29, 2018

rh-atomic-bot commented Oct 30, 2018

jlebon Oct 30, 2018

cgwalters Oct 31, 2018

jlebon commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

cgwalters commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

cgwalters commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

Introduce compose rojig #1512

Introduce compose rojig #1512

Conversation

cgwalters commented Aug 22, 2018

rh-atomic-bot commented Sep 6, 2018

cgwalters commented Sep 7, 2018

cgwalters commented Sep 10, 2018

jlebon commented Sep 10, 2018

cgwalters commented Sep 10, 2018

cgwalters commented Sep 11, 2018

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

rh-atomic-bot commented Sep 18, 2018

cgwalters commented Sep 26, 2018

rh-atomic-bot commented Sep 26, 2018

rh-atomic-bot commented Oct 15, 2018

rh-atomic-bot commented Oct 29, 2018

cgwalters commented Oct 29, 2018

rh-atomic-bot commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

cgwalters commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

cgwalters commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

rh-atomic-bot commented Oct 31, 2018

Introduce `compose rojig` #1512

Introduce `compose rojig` #1512