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

Make composefs a firm dependency #24829

Open
cgwalters opened this issue Dec 12, 2024 · 14 comments
Open

Make composefs a firm dependency #24829

cgwalters opened this issue Dec 12, 2024 · 14 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@cgwalters
Copy link
Contributor

cgwalters commented Dec 12, 2024

Feature request description

Hey so for bootc and other use cases we'd like to get composefs shipped as widely as possible. Today it is a "soft" dependency of podman; things will only blow up if one explicitly enables composefs.

Suggest potential solution

I'd like to discuss making that a "firm" dependency at build time; podman errors out if it is not present by default (at build time), as a forcing function for having it shipped everywhere podman is, in preparation for using it as part of containers/storage.

However, it is possible to omit the requirement by passing --disable-composefs or env DISABLE_COMPOSEFS=1 etc. so to the build system.

Have you considered any alternatives?

We could in theory vendor the C code, but that is less helpful for other users.

Additional context

We have an in-progress Rust crate which is starting to grow its own rewrite of libcomposefs (the C project) because it's not packaged in Debian, etc.

ref https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1064457

@cgwalters cgwalters added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 12, 2024
@Luap99
Copy link
Member

Luap99 commented Dec 12, 2024

I am not to convinced to make it a hard dependency, it is not used when run rootless AFAIK and it is not a default storage setting so user must opt in so I don't quite get the point of why we should require it right now. If a user opts in they can install it as well.
And if it doesn't use composefs why error out?

We could in theory vendor the C code, but that is less helpful for other users.

I don't follow this thought, vendoring code seems to be independent of the dependency type?

Of course if we start using as default it must be a hard dependency.


Also dependency wise podman would not make such call at all. We share the c/storage with buildah and skopeo and more so such packages changes should go into containers-common (in fedora/RHEL land) or whatever the name of the package is on other distros.

@rhatdan
Copy link
Member

rhatdan commented Dec 12, 2024

I would be fine with it being a recommends ,but definitely not a requires.

@cgwalters
Copy link
Contributor Author

I'm talking about a firm requirement; just making build fail by default unless it is present. It'd still be possible to opt-out of having it for now.

I would be fine with it being a recommends ,but definitely not a requires.

I think my use of the term "firm" was probably matching what you meant by "recommends"? I edited my initial comment but again in a nutshell it's:

  • Fail by default at build time if not present
  • You can skip the failure with --disable-composefs or env COMPOSEFS=0 or a Go build tag or whatever.

Also dependency wise podman would not make such call at all. We share the c/storage with buildah and skopeo and more so such packages changes should go into containers-common

OK...should this issue move there then you think?

@cgwalters cgwalters changed the title Make composefs a default-hard dependency Make composefs a firm dependency Dec 12, 2024
@Luap99
Copy link
Member

Luap99 commented Dec 13, 2024

Ok so I misunderstood what you are looking for.
I was thinking you wanted at runtime make podman fail if there is no composefs even if it will not be used.

If the goal instead is to make the build fail if composefs is not installed this seems fine enough. We don't really have a build system besides the standard go build tag support so I guess we would need to use one.

For other storage graph drivers there exists exclude_graphdriver_<name> tags in c/storage already and our Makefile uses some simple check to see if the libs are installed, i.e. https://github.com/containers/podman/blob/main/hack/btrfs_installed_tag.sh

So logicically we could do the same thing for composefs, I have not looked into the c/storage parts but as composefs is not a graph driver on its own so it would be a bit special as the code would need to be separated enough from the normal overlay code so we can use build tags to exclude it.
For other graph drivers AFAIK the build fails with the c libs present as they will be linked via cgo, I am not sure how the dependency looks on composefs? AFAIK we don't link that at all and just call the binary in which case how could we even make the build fail?

cc @giuseppe @mtrmac

Also dependency wise podman would not make such call at all. We share the c/storage with buildah and skopeo and more so such packages changes should go into containers-common

OK...should this issue move there then you think?

I guess if we need build options they must go into c/storage first. Anyway I guess where we discus this is not so important. I am only pointing out that this is not a podman decision IMO but rather a c/storage (or in case of package dependencies a containers-common one)

@sbrivio-rh
Copy link
Collaborator

How widely available is composefs? Now that Debian maintainers have caught up with Podman versions, it would be a bit of a shame to make their life complicated. Podman's CI runs on Debian sid images as well.

@cgwalters
Copy link
Contributor Author

Yes, getting Debian to package composefs is one of the goals of this proposal.

@mtrmac
Copy link
Collaborator

mtrmac commented Dec 13, 2024

My first impression is that this is attempt at a technical fix to a social problem. It’s unexpected to add a dependency purely for the purpose of communicating that this dependency will be required in the future.

If the goal is to tell specifically Debian that this needs to happen, it seems to me that just telling people would achieve the same thing with less annoyance. I would expect a reaction to an artificially-added dependency to be “what a bother, let’s just disable this and worry about placating Podman later — much later if they are going to be adding gratuitous extra work on my plate”, not “or right, I see, I need to package this immediately and I’m grateful for the innovation”, but that’s just me.

Is, perhaps, the thing being solved that there are many downstream distributions and we don’t know how to make them pay attention?


We have an in-progress Rust crate which is starting to grow its own rewrite of libcomposefs (the C project) because it's not packaged in Debian, etc.

(I know ~nothing about the problem space, and I’m perfectly happy to stay out of this conversation. I don’t see how that helps anything. Whether it is a Rust crate or a C library, won’t Debian want to package it independently? In that case, introducing a rewrite would at best mean that Debian will have to package one new package either way; at worst, that they will have to package two things instead of one.

@sbrivio-rh
Copy link
Collaborator

It’s unexpected to add a dependency purely for the purpose of communicating that this dependency will be required in the future.

Oh, my previous understanding was that 1. it's needed right now for proper functionality and 2. that rather than "telling Debian", Debian packages would be introduced and maintained along with this. But now I see it's not the case, so:

If the goal is to tell specifically Debian that this needs to happen, it seems to me that just telling people would achieve the same thing with less annoyance. I would expect a reaction to an artificially-added dependency to be “what a bother, let’s just disable this and worry about placating Podman later — much later if they are going to be adding gratuitous extra work on my plate”, not “or right, I see, I need to package this immediately and I’m grateful for the innovation”, but that’s just me.

...no, it's not just you. As a Debian maintainer, I would do the same if I'm in a good mood and have the time to disable that. There's also the "upstream is making our life harder for no reason so let's stop updating this" possibility.

As Podman's Debian package started recommending passt for pasta(1), I had been maintaining it for a while. I think that alternative would be friendlier to users and other maintainers.

(Same as a Fedora maintainer... the fact that I'll need to maintain three new Fedora packages before AsahiLinux/muvm#111 can be merged is of course delaying everything massively... but I digress)

@cgwalters
Copy link
Contributor Author

My first impression is that this is attempt at a technical fix to a social problem.

Yes.

It’s unexpected to add a dependency purely for the purpose of communicating that this dependency will be required in the future.

But c/storage can be configured to use this today, optionally - it's arguably already a "Recommends" or "Suggests". The goal is just to suggest it a bit more strongly, that's all.

Anyways...dunno, we can let this one sit for a bit and if no one has any strong objections I may take a look at a patch and we can debate the details there.

@mtrmac
Copy link
Collaborator

mtrmac commented Dec 17, 2024

I think it would make sense for a Recommends/Suggests dependency to exist — but adding a technically-unnecessary hard dependency into the codebase would prevent the packages from using a Recommends/Suggests dependency.

IOW, the packagers who did everything right from our perspective, including that they have already packaged composefs, would get a worse packaging result, being forced into a hard requirement when, for users, it isn’t one.

@Luap99
Copy link
Member

Luap99 commented Dec 17, 2024

It is already a recommends from the containers-common package
https://github.com/containers/common/blob/main/rpm/containers-common.spec#L96

@mtrmac
Copy link
Collaborator

mtrmac commented Dec 17, 2024

EDIT: never mind

@Luap99 I understand this proposal to be something vaguely like

// cmd/podman/main.go
func main()
  if composefsNotavailable() { logrus.Fatal("install composefs to use Podman") }
  …
}

@cgwalters
Copy link
Contributor Author

cgwalters commented Dec 17, 2024

No, I never proposed that. I was talking about build time only.

EDIT: Sorry re-reading it I guess that was unclear. I reworded it, does that help?

@mtrmac
Copy link
Collaborator

mtrmac commented Dec 17, 2024

I’m sorry, you did talk about a build-time test later in the issue, I forgot and went back to the first lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

5 participants