Please avoid circular dependency on containers/image

[siretart]

Background: I'm working on packaging buildah, podman and skopeo, and all necessary depednencies, for Debian.

commit a6ca4fc#diff-4061fcef378a6d912e14e2ce162a1995 vendors in containers/image. This creates a circular dependency because containers/image itself vendors containers/storage, which is unfortunate at best. I wonder whether is this really necessary and could imagine that this indicates issues down the road.

Also, I notice that the vendor.conf specification in both packages specify "master". I wonder why that is. Are both packages so severely intertwined that they cannot reasonably be separated in a dependency hierarchy? If that is the case, I wonder whether it wouldn't make more sense to either wait with packaging until both packages have matured more, or package them in the same source package.

[siretart]

@vrothberg and I have discussed the packaging project in depth before. Valentin, can you chime in and provide your perspective on this issue, please?

[vrothberg]

Thanks a lot, @siretart, for bringing up the issue!

I think it's a good example of the differences between golang and traditional Linux packaging. The dependencies in golang are resolved on a package level, not on a project level, which happens quite frequently in the wild. There are other cases in our tools as well but @rhatdan is already on a crusade to untangle buildah and libpod.

In this specific example, we can solve the issue rather easily as manifest.Digest() is a small function. However, I am not sure if we should copy it or if we should move the package into containers/storage. @nalind, @mtrmac WDYT?

Also, I notice that the vendor.conf specification in both packages specify "master". I wonder why that is.

That's an old habit. We pinned the versions in the vendor.conf of Skopeo, Buildah and Podman already but did not perform those changes to c/image and c/storage storage yet. CRI-O is also waiting for it.

Are both packages so severely intertwined that they cannot reasonably be separated in a dependency hierarchy?

It's feasible but not always easy, mostly because golang developers get accustomed to thinking about dependencies on a go-package level. vndr's vendor.conf syntax is hiding that as it allows to specify only projects. The entire golang-dependency management is very painful. go modules will ease some of those pains.

[mtrmac]

I think it's a good example of the differences between golang and traditional Linux packaging. The dependencies in golang are resolved on a package level, not on a project level, which happens quite frequently in the wild.

Yeah, Go does not see c/strorage or c/image quite as an atomic unit.

In a somewhat flippant way of viewing the situation, from a packager’s perspective there’s not that much difference between having three repos (c/image, c/storage, c/manifest) that are packaged into three dpkgs (one for each repo), and two repos (c/image, c/storage) that are packaged into three dpkgs (c/image without c/image/manifest, c/storage, c/image/manifest).

Either way it’s three packages, and either way they need to be kept somewhat in sync because they don’t have a stable API.

So, if it were only the c/image/manifest dependency, I’m not quite sure that splitting it out into a separate repo would be worth it.

---

Unfortunately, this is a bit worse in that c/image/manifest depends on c/image/types (and a few more), and we really want to keep c/image/types in the c/image repo to ease development of c/image, and that in turns makes the packaging of c/image/manifest separately more awkward.

Maybe we do need a ~ c/image/pkg/manifestdigest subpackage without the c/image/types , if the impact on packaging is just big enough; I’m not sure.

In this specific example, we can solve the issue rather easily as manifest.Digest() is a small function. However, I am not sure if we should copy it or if we should move the package into containers/storage. @nalind, @mtrmac WDYT?

It (or rather GuessMIMEType) contains a non-trivial heuristic that I’d much prefer to be maintained in a single place instead of copy&pasted. It’s not big but it could get extremely confusing if different parts of the same process interpreted the same manifest differently.

If it took splitting that functionality out of the c/image/manifest package, or even possibly moving that into to c/storage to make the dependency graph simpler, I’d rather do that than make a copy.

[siretart]

Thanks for taking this concern seriously, I really appreciate your help on getting this resolved.

I understand that in golang, it is easy to specify dependencies on other repositories. That's a great thing, it's exciting to see this making people be more productive and producing great software. But just because something is easy to do, it doesn't necessarily translate to being a good idea. In this particular case, I think circular dependencies may be indicative of suboptimal (poor) organizational choices in the source code. For instance and in this particular case, I don't see how you can possibly keep a consistent set of sources between container/{image,storage,manifest}. This is undesirable at best. It may not manifest in the typical day-to-day work of a golang developer (mostly because the tooling makes this easy, see above), but it does for instance in downstream packaging.

The suggestions to move common code down the hierarchy seem very appropriate. If that initiative is successful, you may think about adding a rule/policy of disallowing circular dependencies.

For continuing my packaging project, I'm exploring the possibility of adding the missing code in form of vendor patches, such as done in other packages (e.g. https://salsa.debian.org/go-team/packages/golang-github-prometheus-tsdb/commit/6e9cea73b962a10ec8dbcd7ea0855b2e1f41c837), but I'm not really thrilled about this option. For the Debian buildds, circular dependencies may not be a showstopper, but make everyone's life unreasonably hard (e.g., bootstrapping new architectures, etc.). I think we (both upstream as well as downstream) would all benefit if they could be avoided in the first place.

[vrothberg]

For continuing my packaging project, I'm exploring the possibility of adding the missing code in form of vendor patches, such as done in other packages (e.g. https://salsa.debian.org/go-team/packages/golang-github-prometheus-tsdb/commit/6e9cea73b962a10ec8dbcd7ea0855b2e1f41c837), but I'm not really thrilled about this option. For the Debian buildds, circular dependencies may not be a showstopper, but make everyone's life unreasonably hard (e.g., bootstrapping new architectures, etc.). I think we (both upstream as well as downstream) would all benefit if they could be avoided in the first place.

@siretart, would it be possible to bundle containers/{image,storage} into one package (e.g., containers-libs)? This would avoid the circular dependency and may reflect the relation between the two repositories a bit more.

[siretart]

We discussed this earlier today.

To answer the question, yes, bundling/merging containers/{image,storage} for packaging would be technically possible, and in fact wouldn't be unprecedented (e.g., in the docker.io packages, cf. https://sources.debian.org/src/docker.io/18.09.1+dfsg1-5/debian/README.source/ for details). However, this would require package maintainers to decide what versions of containers/{image,storage} work well together, and when to update them. I feel that package maintainers are not in a good position to make good calls on this questions, and would strongly recommend upstream to assist here. The most straight-foward way to do so would be to have upstream do product releases that bundle both software packages. This also puts them in a better position with release support lifecycles, and support committments wrt. bug-fixes and security patches.

@vrothberg please feel free to add additional perspective and corrections to my write-up above. Thank you for your time and patience in the phone call earlier today. :-)

[mtrmac]

To answer the question, yes, bundling/merging containers/{image,storage} for packaging would be technically possible, and in fact wouldn't be unprecedented (e.g., in the docker.io packages, cf. https://sources.debian.org/src/docker.io/18.09.1+dfsg1-5/debian/README.source/ for details). However, this would require package maintainers to decide what versions of containers/{image,storage} work well together, and when to update them.

Well, (sadly) package maintainers are already forced to do that, when a single dependency (like the c/image+c/storage bundle being contemplated) is required, using different versions, in its users (buildah, podman, CRI-O, Skopeo). I don’t see that the problem is really any easier or harder when bundling two dependencies our of many (while I appreciate that it may be very hard).

[mtrmac]

In this particular case, I think circular dependencies may be indicative of suboptimal (poor) organizational choices in the source code. For instance and in this particular case, I don't see how you can possibly keep a consistent set of sources between container/{image,storage,manifest}.

That’s a very legitimate concern in general (c/image has so far not made any effort to keep the API stable), but not really a worry for this particular manifest.Digest call; that one is unlikely to break.

This is more indicative of “suboptimal (poor)” design of the manifest digest concept, forcing the storage layer (and transport mechanisms) to understand the manifest format only to compute the digest, even if it otherwise has no need to deal with these quite unrelated concepts.

[And, a bit paradoxically, made worse by c/storage trying fairly hard to keep the API unchanged, which means that it needed to compute the value instead of asking an updated caller to provide it. It seems that constraint will be relaxed in https://github.com//pull/299 . ]

[rhatdan]

Personally I would like to keep these separate, since they are very different code bases and the knowledge of each is required. We basically have different engineering teams concentrating on them.

[rhatdan]

Containers/image dependency in containers/storage has been removed.