Note: this was not detected because unit tests access dependency patches in the spec (which works) but do not check the collection of patches in do_patch (which does not work).

As a consequence of swapping to using Spec.patches, Package.do_patch now uses the same code path to retrieve patches as the patch tests. Arguably adding a test to check that patches have been applied would still be useful.

Was just hit by this in #8207/#8263. GDAL requires a hacked version of JasPer, but the patch wasn't being applied.

This works (checked with flang).

@scheibelp: why didn't the test catch that problem?

turns out Spec.patches already collects patches from the package so the Package.patches_to_apply function isn't necessary. All uses of Package.patches_to_apply are replaced with Package.spec.patches.

The only thing to be careful of is that patches_to_apply returns the patches in the correct order. You'll want to make sure that spec.patches does the same. See #7558 for details. This function was introduced in #7193 but I don't know the internals well enough to understand why it was necessary.

@scheibelp can we merge this some when soon?

The only thing to be careful of is that patches_to_apply returns the patches in the correct order. You'll want to make sure that spec.patches does the same. See #7558 for details. This function was introduced in #7193 but I don't know the internals well enough to understand why it was necessary.

This doesn't actually change anything about the consistency of patch ordering: You are right that #7193 did enforce an order, but that part was reverted in #7558. Patch order is still arbitrary and it remains as unfinished business to enforce the patch declaration order as the consistent ordering - see: #7193 (review)

I wouldn't say the order is arbitrary, my understanding is that the order is that of the patches in the package.py, which seems intuitive. I just want to make sure this behavior is preserved.

my understanding is that the order is that of the patches in the package.py

I think that is agreed upon as what we want to guarantee, but we do not guarantee it right now: Patch ordering has not changed since #7193 (comment) which mentions:

Before this PR we were granted that patch3 would be applied after patch1, but we had no guarantee on the order of application for patch2 with respect to the other two. After this PR we are granted that we apply patches sorted by their path or urls. I think both strategies are wrong and we should apply patches in the order they are declared, if they match the constraint.

That being said, Spec.patches is constructed with Package class patches using the same logic as Package.patches_to_apply. This may seem like duplication of effort, which it is: #7193 was started before merging #5476. In the future, when enforcing the agreed-upon patch order, it will be a matter of modifying the logic which creates Spec.patches.

Can this be merged? Or are there remaining issues to be addressed?

Thanks!