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

rust/treefile: Add basearch key #1766

Closed
wants to merge 4 commits into from
Closed

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Feb 26, 2019

Add a basearch key to the manifest. This can be used at compose time
to assert the architecture the compose is running on. Though my
motivation is for the common case where it gets omitted from the input
manifest and gets automatically added by rpm-ostree into
/usr/share/rpm-ostree/treefile.json for introspection on the client.

(The crucial part here is that the treefile created by rpm-ostree
remains deserializable into a TreeComposeConfig).

Closes: coreos/fedora-coreos-tracker#154

We already had this logic, but it was in `libvm.sh`. Prep for using it
elsewhere.
We're really using this variable to substitute `${basearch}` and find
basearch-specific packages. Let's rename the variable to make that more
obvious.
Add a `basearch` key to the manifest. This can be used at compose time
to assert the architecture the compose is running on. Though my
motivation is for the common case where it gets omitted from the input
manifest and gets automatically added by rpm-ostree into
`/usr/share/rpm-ostree/treefile.json` for introspection on the client.

(The crucial part here is that the treefile created by rpm-ostree
remains deserializable into a `TreeComposeConfig`).

Closes: coreos/fedora-coreos-tracker#154
@cgwalters
Copy link
Member

Nice!

@rh-atomic-bot r+ 92d8f86

@rh-atomic-bot
Copy link

⚡ Test exempted: merge already tested.

rh-atomic-bot pushed a commit that referenced this pull request Feb 27, 2019
We're really using this variable to substitute `${basearch}` and find
basearch-specific packages. Let's rename the variable to make that more
obvious.

Closes: #1766
Approved by: cgwalters
rh-atomic-bot pushed a commit that referenced this pull request Feb 27, 2019
Add a `basearch` key to the manifest. This can be used at compose time
to assert the architecture the compose is running on. Though my
motivation is for the common case where it gets omitted from the input
manifest and gets automatically added by rpm-ostree into
`/usr/share/rpm-ostree/treefile.json` for introspection on the client.

(The crucial part here is that the treefile created by rpm-ostree
remains deserializable into a `TreeComposeConfig`).

Closes: coreos/fedora-coreos-tracker#154

Closes: #1766
Approved by: cgwalters
@jlebon jlebon deleted the pr/base-arch branch February 27, 2019 02:22
@lucab
Copy link
Contributor

lucab commented Mar 13, 2019

I just learned about /usr/share/rpm-ostree/treefile.json here. Is it something in a guaranteed-stable format that external consumers can source without breaking in the future, or is it an internal implementation detail?

@jlebon
Copy link
Member Author

jlebon commented Mar 13, 2019

Is it something in a guaranteed-stable format that external consumers can source without breaking in the future

Yes, it follows the same spec as the input manifest, on which we maintain backwards compatibility. (Though actually... we just "broke" the output format recently by renaming some fields from using underscores to dashes :| ).

We might stop emitting some fields and start emitting new fields as the input treefile itself changes (since we use #[serde(skip_serializing_if = "Option::is_none")] a bunch), but as far as basearch is concerned, it's not going anywhere.

Though this discussion makes me think maybe it wouldn't be a bad idea to start versioning the spec.

@lucab
Copy link
Contributor

lucab commented Mar 15, 2019

I asked because I'd like to depend on this but at the same time I don't want to force the hand on stabilizing the whole struct.

Perhaps another idea is to adopt in a read-only way https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/deployment_guide/sec-using_yum_variables.
That is, having a vars/ somewhere under /usr and expose just some specific single keys in dedicated files there.
It would start with vars/basearch and maybe grow more entries later.
@jlebon thoughts?

@jlebon
Copy link
Member Author

jlebon commented Mar 18, 2019

I asked because I'd like to depend on this but at the same time I don't want to force the hand on stabilizing the whole struct.

Hmm OK, are you thinking of deserializing the whole file as per the TreeComposeConfig definition here even if you just need the basearch field? Whatever we do, we're not going to break this invariant:

(The crucial part here is that the treefile created by rpm-ostree
remains deserializable into a TreeComposeConfig)

Thinking more on this now though, I think I do understand your hesitation a bit: the treefile in the end is only an input file to the compose process. So we're abusing it a bit by using it to derive information about the system it sits on and making it part of the host API. While basearch is likely to remain truthful, there are other things that may no longer be true, e.g. tmp-is-dir or default-target.

It would start with vars/basearch and maybe grow more entries later.

I think that could work, though WDYT about just adding an entry for this in /etc/os-release (really /usr/lib/os-release) since releasever information resides there also?

@lucab
Copy link
Contributor

lucab commented Mar 19, 2019

Discussed out-of-band: while the basearch field/path is pretty stable, there are other things in the JSON schema that may change, so we'd like not to advertise it as an external interface for the moment.

We thus prefer going for a new field (strawman RPM_OSTREE_BASEARCH) in os-release, as part of the mutate-os-release feature of treefile.

As a sidenote, initramfs also has its own os-release (provided by dracut) which may need to be mutated by rpm-ostree.

@cgwalters
Copy link
Member

Is this really specific to rpm-ostree though? Aren't there use cases for getting the rpm basearch inside containers with yum? Any reason not to make this RPM_BASEARCH e.g.?

(Actually implementing it for our container builds is a separate thing though)

@jlebon
Copy link
Member Author

jlebon commented Mar 19, 2019

Prep for this in coreos/fedora-coreos-config#68.

Any reason not to make this RPM_BASEARCH e.g.?

Hmm, though AFAICT, $basearch is not an RPM concept, right? It lives in libdnf. Maybe YUM_BASEARCH instead?

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