search

paketo-buildpacks / libpak

An opinionated extension to the libcnb Cloud Native Buildpack Library
Apache License 2.0
15 stars 17 forks source link

Add support for ARM64 support in DependencyResolver #296

Closed dmikusa closed 10 months ago

dmikusa commented 10 months ago

Summary

This commit adds support to DependencyResolver.Resolve to pick a Dependency and consider the architecture. It looks at either BP_ARCH (if set) or runtime.GOARCH otherwise. It then compares this to the arch= value that is set in PURL. We are using the PURL because it exists already and already contains the arch info. This is opposed to adding a new property to the dependency metadata. We may end up doing that still, but this just gets things working more quickly now and we can change this in the future.

This should be backward compatible. When resolving a dependency, it will read the PURL and pull out the arch.

It then compares this to the "present" value as described above. If the two match, the dependency is kept as a candidate.

Risks

  1. If someone set a PURL for a dep and set an arch, but set it to some incorrect value like foo. It would then use foo and never match. Previously, that arch would have been ignored and things would have work. I assert that this is a problem with the buildpack metadata, much like an invalid PURL. It should be patched and fixed in the buildpack.

  2. ~It's not clear how to handle the situation where there is no ARCH. Like a JAR file. In that case, nothing returned from the dep's PURL arch will ever match the present arch, so that dep won't install properly. I suspect the simplest solution would be to make a special key like all that matches all and use that. It would require a few changes to buildpack dep metadata though.~

Use Cases

Required for ARM64 support.

Checklist

loewenstein commented 10 months ago

Mmh, @dmikusa, idk - we are working on https://github.com/paketo-buildpacks/rfcs/blob/standard-dependency-metadata-format/text/0000-standard-dependency-metadata-format.md to provide that the info separately and agreed that the current purls are not really adequate. I'd rather push that dependency metadata RFC through than build on a metadatum that we should rather remove.

I can see the reasoning to unblock arm64, but as the new metadata is very close to the old one, I do see a chance to find a quick agreement. Once the RFC is in, libpak could quickly start adopting the new arch field.

WDYT?

dmikusa commented 10 months ago

When @anthonydahanne and I talked about this, we didn't want to rush the RFC and we're trying to remove any delays, even if short ones, so we can get ARM64 support moving. There are a lot of people waiting for it, and we feel like we're getting pretty close to having something that's usable for end users.

Our thought is that this PR doesn't make or require any changes to the metadata format, so we can do it now. When the metadata format changes, which we 100% want to see, we can easily update this code to look at the new metadata instead. What the code change looks at is really an implementation detail. Users don't need to care if it's the PURL or an arch field that gets added later. It just needs to filter the dependencies correctly. So we're not trying to start a new way of doing this, but just a short-term fix we can use.

dmikusa commented 10 months ago

It's not clear how to handle the situation where there is no ARCH. Like a JAR file. In that case, nothing returned from the dep's PURL arch will ever match the present arch, so that dep won't install properly. I suspect the simplest solution would be to make a special key like all that matches all and use that. It would require a few changes to buildpack dep metadata though.

I'm proposing we use the absence of the arch= key in the PURL as an indication that it should match any platform. This is consistent with how the Buildpack specification matches targets and archs. https://github.com/buildpacks/spec/blob/main/buildpack.md#targets-1

I made this change, see the latest commit.