Support variant specific patches #485
Conversation
| @@ -98,6 +98,7 @@ def patches_for_requirement( | |||
| patches_dir: pathlib.Path, | |||
| req: Requirement, | |||
| version: Version, | |||
| variant: str = "", | |||
There was a problem hiding this comment.
The variant name should be passed by passing a Context object as first argument.
There was a problem hiding this comment.
That would be the only method with a Context object in this package. This is mostly helpers. And an extra variable with a default value reduces the size of the change.
Not saying I'm against the idea. Just giving more context about why I did it this way.
src/fromager/overrides.py
Outdated
| return itertools.chain( | ||
| # Apply all of the unversioned patches first, in order based on | ||
| # filename. | ||
| sorted(unversioned_patch_dir.glob("*.patch")), | ||
| sorted(unversioned_patch_files), |
There was a problem hiding this comment.
I suggest to sort by the file's name, not the full path.
There was a problem hiding this comment.
Should we be explicit about the precedence of variant-specific patches vs. the general ones by including them in this itertools.chain() call separately?
We probably want
- unversioned
- versioned
- unversioned variant
- versioned variant
right?
There was a problem hiding this comment.
Sounds complicated. I won't be able to recall the correct order in a week from now. How about we throw everything in a single list, then sort all files by their base name?
There was a problem hiding this comment.
We already give precedence to the files in the directory without the version. I think we don't have any of those downstream, so we probably wouldn't break anything making the change you propose. I guess it lets users manage precedence themselves using numerical prefixes, too, so maybe that's a good thing?
There was a problem hiding this comment.
Yes, that was the idea. The prefix is what decides the order. The user can easily insert a variant specific patch by using the same number as the global patch. variant/0001-xx.patch will have precedence over 0002-aa.patch.
fabiendupont
force-pushed
the
allow-variant-specific-patches
branch
from
f0d43f9 to
ef9e36b
Compare
When building wheels for different variants, the code base might be different from one variant to the other, for example when the code comes from a fork with variant specific features. And with a different code base, the patches may fail to be applied. This changes proposes to support patch files stored in a variant specific path under the existing unversioned and versioned patch directories. Below is an example of a patch tree: ``` .../patches .../patches/deepspeed .../pacthes/deepspeed/0001-unversioned.patch .../pacthes/deepspeed/0002-unversioned.patch .../patches/deepspeed/brie/0001-unversioned.patch .../patches/deepspeed/feta/0002-unversioned.patch .../patches/deepspeed-0.5.0/0001-versioned.patch .../patches/deepspeed-0.5.0/brie/0003-versioned.patch ``` When applying the patches for the `brie` variant, the following patches would be applied: ``` .../pacthes/deepspeed/0001-unversioned.patch .../patches/deepspeed/brie/0001-unversioned.patch .../pacthes/deepspeed/0002-unversioned.patch .../patches/deepspeed-0.5.0/0001-versioned.patch .../patches/deepspeed-0.5.0/brie/0003-versioned.patch ``` When applying the patches for the `feta` variant, the following patches would be applied: ``` .../pacthes/deepspeed/0001-unversioned.patch .../pacthes/deepspeed/0002-unversioned.patch .../patches/deepspeed/feta/0002-unversioned.patch .../patches/deepspeed-0.5.0/0001-versioned.patch ``` Signed-off-by: Fabien Dupont <[email protected]>
fabiendupont
force-pushed
the
allow-variant-specific-patches
branch
from
ef9e36b to
ae2774a
Compare
|
Seems that #488 is also implementing this change. @tiran, @dhellmann, I guess this PR can be closed, right? |
I think so, yes. |
|
Closing in favor of #488 |
When building wheels for different variants, the code base might be
different from one variant to the other, for example when the code comes
from a fork with variant specific features. And with a different code
base, the patches may fail to be applied.
This changes proposes to support patch files stored in a variant
specific path under the existing unversioned and versioned patch
directories. Below is an example of a patch tree:
When applying the patches for the
brievariant, the following patcheswould be applied:
When applying the patches for the
fetavariant, the following patcheswould be applied:
Fixes #486