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

packaging/rpm-ostree.spec: Update to sync with rawhide #5047

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

jmarrero
Copy link
Member

The specs between upstream, fedora and centos are out of sync. This attempts to sync it to restart using it as the source of truth.

@jmarrero jmarrero requested a review from jlebon August 14, 2024 15:47
@@ -132,12 +138,11 @@ Requires: librepo%{?_isa} >= %{librepo_version}
# rpm-ostree wraps more of ostree (such as `ostree admin unlock` etc.)
Requires: ostree
Requires: bubblewrap
Requires: fuse
Requires: fuse3
Copy link
Member

Choose a reason for hiding this comment

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

This seems to come from https://src.fedoraproject.org/rpms/rpm-ostree/c/3c602a23787fd2df873c0b18df3133c9fec4b66a, but the reasoning there is not sufficient.

OSTree already lists its fuse dependencies as needed. git archeology shows the fuse here is because we call fusermount -u: #443.

fuse3 ships fusermount3 and I'm not sure if they're supposed to be interchangeable for unmount purposes (though I would assume it mostly boils down to a umount() syscall) but clearly using fusermount -u to unmount a FUSE mount from rofiles-fuse (compiled with libfuse3) has been working for a while, ever since we started compiling ostree in Fedora against libfuse3.

Hmm, wonder if this is the root cause of coreos/coreos-assembler#3848 somehow; maybe that mismatch stopped working recently?

Anyway, I think ideally there'd be an rofiles-fuse -u which would just proxy to the right fusermount vs fusermount3 based on compiled options. Barring that, in practice I think this is fine, but we should probably change our code to use fusermount3 -u also.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, wonder if this is the root cause of coreos/coreos-assembler#3848 somehow; maybe that mismatch stopped working recently?

At least testing locally with latest fuse bits, it seems to work fine to unmount with fusermount -u an rofiles-fuse mount, so maybe not.

But still, we need the Requires here to agree with the actual binary we use in the code.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely a mess here...a perfect example of the pitfalls of trying to maintain spec files upstream.

A larger fix of course is for us to just stop using rofiles-fuse (and go with overlayfs, but...)

Copy link
Member

@travier travier Aug 20, 2024

Choose a reason for hiding this comment

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

Won't this break if we explicitly call fusermount as the fuse3 package does not provide such a binary?

We should likely revert that change until we've moved the logic in rpm-ostree to try both fuse2 and fuse3 (or add a wrapper).

Copy link
Member

Choose a reason for hiding this comment

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

Won't this break if we explicitly call fusermount as the fuse3 package does not provide such a binary?

Yup, this is what I meant higher up with:

we need the Requires here to agree with the actual binary we use in the code.

I think rather than reverting, it makes more sense to just plow forward and move over to the v3 version. Pushed a commit for that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely a mess here...a perfect example of the pitfalls of trying to maintain spec files upstream.

Yeah, I'd be OK revisiting that. Historically, the canonical version lived here because it often required changes along with upstream changes (e.g. bundled libdnf deps). Since now things are calmer, it might make sense to just move it back to dist-git.

jlebon added a commit to jmarrero/rpm-ostree that referenced this pull request Aug 20, 2024
rofiles-fuse is now built against libfuse3 on all the platforms we care
about in rpm-ostree. Accordingly also switch over the fusermount utility
we use to the v3 version to match. This also allows composes to drop out libfuse2 if
nothing else pulls it in.

See also: coreos#5047 (comment)
jlebon added a commit to jmarrero/rpm-ostree that referenced this pull request Aug 20, 2024
rofiles-fuse is now built against libfuse3 on all the platforms we care
about in rpm-ostree. Accordingly also switch over the fusermount utility
we use to the v3 version to match. This also allows composes to drop out
libfuse2 if nothing else pulls it in.

See also: coreos#5047 (comment)
@@ -268,5 +273,4 @@ fi
%files devel -f files.devel

%changelog
* Thu Nov 17 2022 Colin Walters <walters@verbum.org> - 2022.15-3
- Dummy change to satisfy rpm timestamp clamping
%autochangelog
Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember that breaking some things with copr repos. Might have to look if this works with our continuous copr builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see this comment:
https://fedoramagazine.org/use-rpmautospec-in-fedora-linux/#comment-550158

from a year ago hopefully now it's good 🤞

@jlebon
Copy link
Member

jlebon commented Aug 28, 2024

I think this is actually needed now to unbreak CI: #5069 (comment)

@jmarrero
Copy link
Member Author

jmarrero commented Aug 28, 2024

I think this is actually needed now to unbreak CI: #5069 (comment)

I just changed it back to fuse then. Let me know if you think it needs any other changes.
or are you saying we need fuse 3?

@cgwalters
Copy link
Member

Ohhh I finally see now:

warning: Found 1 packages to download in cache-only mode

OK and we're actually explicitly turning off network. Now I understand this isn't just a flake. Hmm, we should clearly make that a hard error instead, at least for the cosa case. Adding --cache-only-strict or something.

@cgwalters
Copy link
Member

I'm so confused by this though still...we still Requires: fuse in f40, which is what we're trying to build here right? Ohh I think I see, the fuse requirement change actually changed in f40 sometime post-"GA" of f40. Probably because we merged git branches? Ugh...

Anyways yes I see:
https://github.com/coreos/fedora-coreos-config/blob/f5ea8ce3c5b2fcc23aca646885ceaae134936e48/manifest-lock.x86_64.json#L717

So I guess yes we just need to update to fuse3 here.

@jlebon
Copy link
Member

jlebon commented Aug 29, 2024

or are you saying we need fuse 3?

Yup, I had pushed a commit to do that. I'm saying we should match the libfuse version that rofiles-fuse uses.

@cgwalters
Copy link
Member

I don't see an updated commit?

@cgwalters
Copy link
Member

OK though what really needs to be done here is to upstream make the spec use distribution conditionals, which is what we should have done from the start to help us maintain a single spec

@cgwalters
Copy link
Member

Hmm do we really even need this dependency at all? Today the ostree package has rofiles-fuse and ostree has the dep...

@cgwalters
Copy link
Member

OK my MRs-for-dist-git aren't set up, any objections to me pushing this to fedora and here?

From 92516fb809e96ed6a29f35d7aad0619add63b972 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Thu, 29 Aug 2024 10:01:55 -0400
Subject: [PATCH] Drop `Requires: fuse3`

We already have this transitively through the `Requires: ostree`
and dealing with fuse-vs-fuse3 is a pain; cc
https://github.com/coreos/rpm-ostree/pull/5047
---
 rpm-ostree.spec | 1 -
 1 file changed, 1 deletion(-)

diff --git a/rpm-ostree.spec b/rpm-ostree.spec
index 10d4c79..a2d6cb5 100644
--- a/rpm-ostree.spec
+++ b/rpm-ostree.spec
@@ -136,7 +136,6 @@ Requires:       librepo%{?_isa} >= %{librepo_version}
 # rpm-ostree wraps more of ostree (such as `ostree admin unlock` etc.)
 Requires: ostree
 Requires: bubblewrap
-Requires: fuse3
 
 # For container functionality
 # https://github.com/coreos/rpm-ostree/issues/3286
-- 
2.44.0

@jmarrero
Copy link
Member Author

lgtm, :shipit: I'll update it here.

@jlebon
Copy link
Member

jlebon commented Aug 29, 2024

Had a chat with @jmarrero and @cgwalters on this. There was agreement that ideally, there'd be an rofiles-fuse -u. That said for now, we will:

  • change the spec file to be conditional: on el9, we Requires: fuse, everywhere else, we Requires: fuse3. We also flip a build-time conditional based on that same spec condition.
  • change the code to use this build time conditional to know if to call fusermount or fusermount3.

@cgwalters
Copy link
Member

There was agreement that ideally, there'd be an rofiles-fuse -u.

Maybe...but hold up, I think this may just be Fedora packaging fuse in a broken way. At least looking at e.g. ubuntu, they have a lib fuse for each of 2 and 3 but only a single fusermount binary in https://packages.ubuntu.com/focal/fuse and I would bet money that it's totally compatible to run fusermount (linked to v2) against a mountpoint created with libfuse3.

@cgwalters
Copy link
Member

jmarrero added a commit to jmarrero/rpm-ostree that referenced this pull request Aug 30, 2024
We have been building with fuse but changed to fuse3  on:
https://src.fedoraproject.org/rpms/rpm-ostree/c/3c602a23787fd2df873c0b18df3133c9fec4b66a
However our code is just calling fuse's fusermount.
We are updating our spec and code based on the discusion on:
coreos#5047

Co-authored-by: Colin Walters <walters@verbum.org>
@HuijingHei
Copy link
Member

I am confused why the CI failed with can not download fuse, but IMU we do not need it for fedora build, see #5047 (comment)

[2024-08-29T19:40:05.618Z] warning: Found 1 packages to download in cache-only mode

[2024-08-29T19:40:05.618Z] Downloading from 'fedora'...done

[2024-08-29T19:40:05.618Z] error: Installing packages: Downloading from 'fedora': Cannot download Packages/f/fuse-2.9.9-21.fc40.x86_64.rpm: All mirrors were tried; Last error: Curl error (6): Couldn't resolve host name for https://dl.fedoraproject.org/pub/fedora/linux/releases/40/Everything/x86_64/os/Packages/f/fuse-2.9.9-21.fc40.x86_64.rpm [Could not resolve host: dl.fedoraproject.org]

[2024-08-29T19:40:08.123Z] failed to execute cmd-build: exit status 1

# However our code is just calling fuse's fusermount.
# We are updating our spec and code based on the discusion on:
# https://github.com/coreos/rpm-ostree/pull/5047
%if 0%{?el} <= 9
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this conditional is right...I suspect it's evaluating to true on fedora too and that's why we're hitting this bug still.

I think it may need to be e.g.

%if 0%{?rhel} && 0%{?el} <= 9

or so?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/search?q=+0%25%7B%3Fel%7D&type=code

I see people doing:

%if 0%{?el} >= 6 || 0%{?rhel} >= 6

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, trying your conditional in this push.

Copy link
Member Author

Choose a reason for hiding this comment

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

something is still weird with the conditional:
https://gitlab.com/redhat/centos-stream/rpms/rpm-ostree/-/merge_requests/57#note_2093239477

I don't get why... maybe switching this around to:

%if 0%{?fedora} || 0%{?rhel} > 9 || 0%{?el} > 9
Requires: fuse3
%else
Requires: fuse
%endif

Would work... I am trying to find another rpm with an example.

Copy link
Member Author

@jmarrero jmarrero Sep 6, 2024

Choose a reason for hiding this comment

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

Apparently 0%{?rhel} should match the family of distros based on this: https://docs.fedoraproject.org/en-US/packaging-guidelines/DistTag/#_conditionals I wonder if the el conditional is throwing it.

Copy link
Member

Choose a reason for hiding this comment

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

%if 0%{?fedora} || 0%{?rhel} > 9 || 0%{?el} > 9

Check on centos stream 9, get %{?rhel} -> 9, but get %{?el} null. IMU, the condition LGTM.

$ podman run --rm -it quay.io/centos/centos:stream9 bash
[root@b7210efdfe5a /]# rpm --eval %{?rhel}
9
[root@b7210efdfe5a /]# rpm --eval %{?el}

jmarrero added a commit to jmarrero/rpm-ostree that referenced this pull request Sep 3, 2024
We have been building with fuse but changed to fuse3  on:
https://src.fedoraproject.org/rpms/rpm-ostree/c/3c602a23787fd2df873c0b18df3133c9fec4b66a
However our code is just calling fuse's fusermount.
We are updating our spec and code based on the discusion on:
coreos#5047

Co-authored-by: Colin Walters <walters@verbum.org>
jmarrero and others added 2 commits September 3, 2024 14:08
The specs between upstream, fedora and centos are out
of sync. This attempts to sync it to restart using it
as the source of truth.
We have been building with fuse but changed to fuse3  on:
https://src.fedoraproject.org/rpms/rpm-ostree/c/3c602a23787fd2df873c0b18df3133c9fec4b66a
However our code is just calling fuse's fusermount.
We are updating our spec and code based on the discusion on:
coreos#5047

Co-authored-by: Colin Walters <walters@verbum.org>
@cgwalters cgwalters merged commit 3c1bcef into coreos:main Sep 3, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants