Only enforce strict arch checking when dependency specifies arch

[jericop]

Summary

This PR does the following:

• Disables strict dependency checking based on architecture
• Only set BP_ARCH for unit tests related to dependencies with architecture

this relates to paketo-buildpacks/packit#671 and will ensure consistency between packit and libpak

Use Cases

The 0059-standard-dependency-metadata-format RFC says that when os and arch are not given it should be assumed that the dependency is compatible with any architecture. This will be a requirement for some buildpacks such as pip and yarn, which work on any architecture.

This PR makes it so that when dependencies specify arch they can only be used on systems with the specified arch or if env BP_ARCH is set to match. This means strict checking is only enabled when dependencies specify arch.

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]

I worry about this being confusing. In once place we're looking at the result of c.GetArch() and in the other we're looking at just c.Arch. It seems like we should look at one consistently.

Also, looking directly at c.Arch will not take into consideration buildpacks that have yet to add arch metadata, which is basically every buildpack that is using libpak. That work hasn't been done. I think that's going to cause a problem or least an inconsistency in behavior. In that case, `c.Arch != "" will always be false because it'll always be "", so it'll never skip that dependency.

[dmikusa]

Is there a reason you opted to not modify the logic in c.GetArch()?

[dmikusa]

In hindsight...

func archFromPURL(rawPURL string) (string, error) {
	if len(strings.TrimSpace(rawPURL)) == 0 {
		return "amd64", nil
	}

	purl, err := url.Parse(rawPURL)
	if err != nil {
		return "", fmt.Errorf("unable to parse PURL\n%w", err)
	}

	queryParams := purl.Query()
	if arch, ok := queryParams["arch"]; ok {
		return arch[0], nil
	}

	return archFromSystem(), nil
}

The last bit of this looks weird to me. I'm not sure why we'd take the archFromSystem() and return that for what is supposed to represent the arch of the dependency. Maybe that was an attempt to make dependencies with no arch always match? Because it would return the system arch and the comparison on line 305 is arch != archFromSystem()?

[jericop]

Initially I was going to use GetArch, but then found that it wasn't just returning the Arch value of the dependency and was instead calling archFromPURL and then archFromSystem. I didn't think it made sense to bury the logic in those functions. It could potentially benefit from a bit of refactoring, but I was trying to minimize changes and avoid potentially breaking existing buildpacks.

Ultimately I was trying to make it clear that Arch is only checked with Arch is not empty.

[dmikusa]

I'm not sure this is safe. There are tests that depend on the arch to operate consistently on an M1 or x86 system. Did you run your tests on different arch systems?

[jericop]

I have an M1 mac and ran the unit tests locally without issue. It looks like the runner used by GitHub actions is x86, so that should cover both architectures.

[dmikusa]

Marking as a bug because the current implementation does not follow the RFC.

I do worry about this being a breaking change and if this is the right time to make this change. Since we haven't updated metadata across buildpacks yet. I think this would be simpler if we had updated all of the metadata to use the new fields. Then we could actually remove the logic about checking the PURL and simplify this even more.

[jericop]

Marking as a bug because the current implementation does not follow the RFC.

I do worry about this being a breaking change and if this is the right time to make this change. Since we haven't updated metadata across buildpacks yet. I think this would be simpler if we had updated all of the metadata to use the new fields. Then we could actually remove the logic about checking the PURL and simplify this even more.

I agree with marking this as a bug so the implementation matches the RFC (eventually). I'm currently working on buildpacks that use packit, so this is not blocking me in any way. My primary motivation with this PR was to be consistent with the RFC.