Skip to content
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

app/compose: include rpmdb pkglist in compose #1134

Closed
wants to merge 3 commits into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Dec 5, 2017

We don't want to have to download all of /usr/share/rpm just to get
the list of packages used to compose the tree. This is fundamental
information that needs to be easier to discover. So let's stick it right
in the commit metadata. There's various use cases for this information,
including easily checking for and displaying updates and a pkglist-aware
version of ostree log.

gv_nevra_from_pkg (DnfPackage *pkg)
{
return g_variant_ref_sink (g_variant_new ("(sstsss)",
dnf_package_get_nevra (pkg),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refresh my memory why we have both the composed/decomposed versions of this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the overrides case, it's really just to make it easier to consume the output of --json in tests. Since bandwidth is a consideration here, I think we can just reassemble it client-side. Will fix up!

return FALSE;

GVariantBuilder pkglist_v_builder;
g_variant_builder_init (&pkglist_v_builder, (GVariantType*)"av");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a(stss) or so - the individual variants shouldn't vary.

@jlebon
Copy link
Member Author

jlebon commented Dec 6, 2017

Fix up and test! ⬆️

@cgwalters
Copy link
Member

LGTM.

@dustymabe
Copy link
Member

interesting. Does this have implications for #558 and ostreedev/ostree#954 ?

@jlebon
Copy link
Member Author

jlebon commented Dec 7, 2017

Does this have implications for #558 and ostreedev/ostree#954 ?

Should make it much easier, yes. Though if we want to support history prior to commits that do not have this metadata, then it complicates things. We can discuss that in the issue!

@jlebon
Copy link
Member Author

jlebon commented Dec 7, 2017

Let's see how the infra is today.
@rh-atomic-bot r=cgwalters 2f7e722

@rh-atomic-bot
Copy link

⌛ Testing commit 2f7e722 with merge 409740a...

rh-atomic-bot pushed a commit that referenced this pull request Dec 7, 2017
We should be returning `FALSE` here, not `EXIT_FAILURE`.

Closes: #1134
Approved by: cgwalters
rh-atomic-bot pushed a commit that referenced this pull request Dec 7, 2017
Basically, it doesn't make sense for the caller to only want the
pkglist, but not the refsack because the former has a more limited
lifetime than the latter. Check for that to make sure nobody falls in
this trap like I did.

Closes: #1134
Approved by: cgwalters
rh-atomic-bot pushed a commit that referenced this pull request Dec 7, 2017
We don't want to have to download all of `/usr/share/rpm` just to get
the list of packages used to compose the tree. This is fundamental
information that needs to be easier to discover. So let's stick it right
in the commit metadata. There's various use cases for this information,
including easily checking for and displaying updates and a pkglist-aware
version of `ostree log`.

Closes: #1134
Approved by: cgwalters
@dustymabe
Copy link
Member

Though if we want to support history prior to commits that do not have this metadata, then it complicates things.

I'm not too worried about that. Onward and upward!

@rh-atomic-bot
Copy link

💔 Test failed - status-atomicjenkins

@jlebon
Copy link
Member Author

jlebon commented Dec 7, 2017

Oh heh, we don't actually have rpm-ostree in the test compose. I swapped that check in for systemd! ⬆️

@jlebon
Copy link
Member Author

jlebon commented Dec 8, 2017

bot, retest this please

@cgwalters
Copy link
Member

@rh-atomic-bot r+ acee00c

@rh-atomic-bot
Copy link

🔒 Merge conflict

We should be returning `FALSE` here, not `EXIT_FAILURE`.
Basically, it doesn't make sense for the caller to only want the
pkglist, but not the refsack because the former has a more limited
lifetime than the latter. Check for that to make sure nobody falls in
this trap like I did.
We don't want to have to download all of `/usr/share/rpm` just to get
the list of packages used to compose the tree. This is fundamental
information that needs to be easier to discover. So let's stick it right
in the commit metadata. There's various use cases for this information,
including easily checking for and displaying updates and a pkglist-aware
version of `ostree log`.
@jlebon jlebon force-pushed the pr/rpmdb-in-commit-metadata branch from acee00c to e11345d Compare December 8, 2017 17:02
@jlebon
Copy link
Member Author

jlebon commented Dec 8, 2017

Rebased! ⬆️

@cgwalters
Copy link
Member

@rh-atomic-bot r+ e11345d

@rh-atomic-bot
Copy link

⚡ Test exempted: pull fully rebased and already tested.

rh-atomic-bot pushed a commit that referenced this pull request Dec 8, 2017
Basically, it doesn't make sense for the caller to only want the
pkglist, but not the refsack because the former has a more limited
lifetime than the latter. Check for that to make sure nobody falls in
this trap like I did.

Closes: #1134
Approved by: cgwalters
rh-atomic-bot pushed a commit that referenced this pull request Dec 8, 2017
We don't want to have to download all of `/usr/share/rpm` just to get
the list of packages used to compose the tree. This is fundamental
information that needs to be easier to discover. So let's stick it right
in the commit metadata. There's various use cases for this information,
including easily checking for and displaying updates and a pkglist-aware
version of `ostree log`.

Closes: #1134
Approved by: cgwalters
@jlebon jlebon deleted the pr/rpmdb-in-commit-metadata branch February 15, 2018 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants