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

Add support for ARM64 support in DependencyResolver #296

Merged
merged 2 commits into from
Nov 15, 2023
Merged

Conversation

dmikusa
Copy link
Contributor

@dmikusa dmikusa commented Oct 29, 2023

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.

  • If the PURL is not set, it defaults to AMD64 which is the present behavior
  • If the PURL is set but does not have an arch=, it defaults to AMD64 which is the present behavior
  • If the PURL is malformed, it will error. That will be a difference in behavior, but should not happen unless something is wrong with the metadata.
  • If the PURL is set and has an arch, then it'll return that value.

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

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@dmikusa dmikusa added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Oct 29, 2023
@dmikusa dmikusa requested a review from a team as a code owner October 29, 2023 04:59
@dmikusa dmikusa marked this pull request as draft October 29, 2023 05:00
@loewenstein
Copy link

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
Copy link
Contributor Author

dmikusa commented Oct 29, 2023

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
Copy link
Contributor Author

dmikusa commented Oct 30, 2023

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.

@dmikusa dmikusa marked this pull request as ready for review October 30, 2023 02:34
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.

Signed-off-by: Daniel Mikusa <[email protected]>
If a PURL does not have an arch= key, then we will assume it applies to any arch. This would be for JAR files or other non-architecture dependent packages. This behavior is slightly different than when there is no PURL at all. If there is no PURL at all, then we assume x86. This is for backwards compatibility, for buildpacks that do not include PURLs.

Signed-off-by: Daniel Mikusa <[email protected]>
Copy link
Member

@anthonydahanne anthonydahanne left a comment

Choose a reason for hiding this comment

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

@anthonydahanne anthonydahanne merged commit ee5dde1 into main Nov 15, 2023
4 checks passed
@anthonydahanne anthonydahanne deleted the arm64 branch November 15, 2023 03:04
dmikusa added a commit that referenced this pull request Nov 11, 2024
Signed-off-by: Daniel Mikusa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants