| Summary: | [Future EAPI] Introduce patch applying function | ||
|---|---|---|---|
| Product: | Gentoo Hosted Projects | Reporter: | Michał Górny <mgorny> |
| Component: | PMS/EAPI | Assignee: | PMS/EAPI <pms> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | esigra, hasufell, hristo, jlec, pacho |
| Priority: | Normal | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| See Also: | https://bugs.gentoo.org/show_bug.cgi?id=370701 | ||
| Whiteboard: | in-eapi-6 | ||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | |||
| Bug Blocks: | 174380, 463692, 475288 | ||
|
Description
Michał Górny
2013-03-29 18:07:39 UTC
doesn't epatch also take "-p*" successfully as argument? (In reply to comment #0) requiring people to specify the -p# level for every patch is not an advantage (In reply to comment #1) yes, `epatch` automatically takes all -* args on a per-patch basis, not just -p# (In reply to comment #2) > (In reply to comment #0) > > requiring people to specify the -p# level for every patch is not an advantage I was rather thinking of requiring them to supply proper patches suitable for -p1. It's not so hand and makes their future a little brighter. that will not work as a drop-in replacement and I think it should (In reply to comment #4) right. we have a lot of patches that we don't write but instead we fetch from other distros (Debian/Fedora/SUSE/etc...), or from 3rd parties which add functionality (like qmail or openssh or ...). forcing a -p1 mentality there simply won't work. i agree that for Gentoo-written patches, -p1 is good hygiene. but in the end, what does forcing it on all files *actually* gain us ? you've mentioned two minor downsides to the epatch way in comment #0, but all the gains epatch gets us far out-weigh those two minor nuisances. (In reply to comment #4) > that will not work as a drop-in replacement and I think it should We can't afford a drop-in replacement. epatch grow into a little monster, with almost 300 lines of code. You won't fit that into PMS. (In reply to comment #6) > We can't afford a drop-in replacement. epatch grow into a little monster, > with almost 300 lines of code. You won't fit that into PMS. Of course it has grown with time. What else would you expect for a function that provides such a central piece of functionality that is used by almost every ebuild? Either we should move _all_ of it to PMS, or we should leave it in the eclass where it is (together with the things that depend on it, like epatch_user). It doesn't make any sense to have the package manager provide only a crippled version of it. I'm not so sure that most of its functionality is useful functionality. There's a lot there that seems to have been added "because we can". Maybe forcing maintainers to provide cleaner patches is a good thing... FWIW, Exherbo's been doing that (forcing -p1, etc), and it's not a problem. (In reply to comment #7) > Of course it has grown with time. What else would you expect for a function > that provides such a central piece of functionality that is used by almost > every ebuild? I would expect it to be better maintained, to be honest. Just adding more and more features to wrapper whose sole responsibility is applying patches doesn't sound that right to me. Seriously, how many of the functions are you using? How much of it people use just because it is there, though it could be handled another way? > Either we should move _all_ of it to PMS, or we should leave it in the > eclass where it is (together with the things that depend on it, like > epatch_user). It doesn't make any sense to have the package manager provide > only a crippled version of it. I was thinking of either putting a sane patch applying function in PMS or just providing the sane details necessary to handle default src_prepare() like suggested. If people really need all of it, they can still use bloated epatch. (In reply to comment #8) > I'm not so sure that most of its functionality is useful functionality. > There's a lot there that seems to have been added "because we can". Maybe > forcing maintainers to provide cleaner patches is a good thing... FWIW, > Exherbo's been doing that (forcing -p1, etc), and it's not a problem. I have to agree here. I think we're applying the fix to the wrong problem. To be honest, I don't really mind the whole -p0..-p5 thing. Although I would rather let PM maintainers decide themselves how to find out the correct value rather than enforcing the eclass way. And I dislike putting random numbers like '-p5' here in the PMS. While I don't mind it, I don't feel like it is really necessary either. If having patches with random patch-levels is a problem, I think a better solution to it would be having a tool to fix patches. Having fixed patches will benefit us all, without increasing the workload much. A completely wrong feature IMO is uncompressing patches. Seriously, where should it be used? I don't think we really want to keep compressed patches in FILESDIRs. We rather host them, and then 'unpack' becomes responsible for uncompressing them. And while unbloating it, I would drop recursive patching as well. That one finds a few uses but requires just more and more features (suffixes, includes, excludes...). Practically, most of it can be achieved with globs. (In reply to comment #9) > To be honest, I don't really mind the whole -p0..-p5 thing. Although I would > rather let PM maintainers decide themselves how to find out the correct > value rather than enforcing the eclass way. If we "let PM maintainers decide" then we're going to have cases where a given patch is accepted by one package manager while it's rejected by another. (In reply to comment #9) of course everything epatch does can be accomplished by people doing it themselves. but that defeats the entire point of having a centralized function that "just works" where you can feed it a directory or a series of patches. having a tool to "fix" patches is pointless because we already have a tool that accepts them regardless of the input. the decompressing logic makes more sense when used for bulk patching. people compress patches then tar them up then apply them. this used to be a lot more common in the past, but that style has been discouraged, so it has fallen significantly. glob's don't just work because the bulk apply supports $ARCH specific patches. this is another feature that was used heavily in the past, but has been discouraged in recent times. similarly, people can exclude patches easily by glob. that doesn't mean these features need to be carried across. if your only goal is to support a global PATCHES array that gets applied by the default src_prepare default, then there's no actual need to design/export a custom patch command. it's easy to complain about features you don't like and you don't think make sense when you don't bother looking at the historical context and original designs. (In reply to comment #10) > (In reply to comment #9) > > To be honest, I don't really mind the whole -p0..-p5 thing. Although I would > > rather let PM maintainers decide themselves how to find out the correct > > value rather than enforcing the eclass way. > > If we "let PM maintainers decide" then we're going to have cases where a > given patch is accepted by one package manager while it's rejected by > another. You're overreacting here. I think we can rely on package manager maintainers to know what they're doing without guiding them by hand. The general idea is that you can basically find proper -p without actually calling 'patch' a dozen of times and guessing based on its errors. I'd rather see a PM take paths from the patch file, guess based on them and then run 'patch' just once. And do not hide its output. (In reply to comment #11) > (In reply to comment #9) > > of course everything epatch does can be accomplished by people doing it > themselves. but that defeats the entire point of having a centralized > function that "just works" where you can feed it a directory or a series of > patches. > > having a tool to "fix" patches is pointless because we already have a tool > that accepts them regardless of the input. That's what I was talking about with fix to the wrong problem. The problem is that patches are ugly, broken and hardly useful; not that 'patch' program doesn't apply them cleanly. Fixing the patches makes them useful for everything, 'epatch' just lets you hopefully apply them in an ebuild. Don't clean up when you already got used to the odour? > the decompressing logic makes more sense when used for bulk patching. > people compress patches then tar them up then apply them. this used to be a > lot more common in the past, but that style has been discouraged, so it has > fallen significantly. And why are we talking about the past and discouraged stuff? We're trying to get a function into *new* EAPI. There's no point in supporting ancient practices there, especially when they were obviously proven wrong. > glob's don't just work because the bulk apply supports $ARCH specific > patches. this is another feature that was used heavily in the past, but has > been discouraged in recent times. Yes, it is 'highly discouraged'. Why should we have it then? > it's easy to complain about features you don't like and you don't think make > sense when you don't bother looking at the historical context and original > designs. The historical context and original design should only matter not to repeat past mistakes when creating something new. (In reply to comment #12) i don't see how implementing `patch --dry-run` in the PM is any better than running patch directly. sounds like pointless work. i really don't follow your logic about "stinky patches". wtf cares. if it applies cleanly, then that's fine. forcing -p1 might make people "feel better", but in practice, there's no *actual* benefit. i never said you had to support all the functionality epatch has. i pointed out that these things are actually useful and it's better to have a system that handles these details for you rather than open coding them constantly yourself (i.e. `unpack` and `rm` and `mv` and ...). i did say ask why you even need a dedicated patch command. you didn't cite a reason. since PATCHES=() would be a variable that the default src_prepare would process, then there's no need for people to run `patch` themselves. if they do actually want to apply patches by hand, then they can use `epatch`. It's not only the PATCHES array, but also epatch_user which was suggested for EAPI 5 already (and was rejected because nobody had an idea how it should be implemented). I think for epatch_user one would want autodetection of the -p* depth in any case. (In reply to comment #13) > (In reply to comment #12) > > i don't see how implementing `patch --dry-run` in the PM is any better than > running patch directly. sounds like pointless work. I'm not saying about re-implementing it all. But in fact, doing so will for example allow to distinguish read/write errors from patches not applying cleanly. > i really don't follow your logic about "stinky patches". wtf cares. if it > applies cleanly, then that's fine. forcing -p1 might make people "feel > better", but in practice, there's no *actual* benefit. Unless that's a patch someone will use elsewhere. epatch is fun but it's not available outside of ebuild environment by default. And the patches are -- people may need to apply them or submit them to some kind of bug tracker. They're on their own, fighting 'patch -pX' because *you* were too lazy to fix it. > i did say ask why you even need a dedicated patch command. you didn't cite > a reason. since PATCHES=() would be a variable that the default src_prepare > would process, then there's no need for people to run `patch` themselves. > if they do actually want to apply patches by hand, then they can use Comment #1 states the reason. PMS already does some things without definining the functions it uses for them -- and that's an awful inconsistency. You're saying about having automatically applied patches but not being able to apply patches manually without resorting to the eclass. (In reply to comment #14) > It's not only the PATCHES array, but also epatch_user which was suggested > for EAPI 5 already (and was rejected because nobody had an idea how it > should be implemented). I think for epatch_user one would want autodetection > of the -p* depth in any case. Agreed. I believe that the main point of this bug is decide what we need in PMS. So far, I see that we just need support for regular files and -pX guessing. Is there something else that could benefit PATCHES=() and epatch_user? I'll grep the tree a bit. (In reply to comment #15) > I believe that the main point of this bug is decide what we need in PMS. So > far, I see that we just need support for regular files and -pX guessing. Is > there something else that could benefit PATCHES=() and epatch_user? Support for directories, including recognition of ???_arch_foo.patch. (In reply to comment #16) > (In reply to comment #15) > > I believe that the main point of this bug is decide what we need in PMS. So > > far, I see that we just need support for regular files and -pX guessing. Is > > there something else that could benefit PATCHES=() and epatch_user? > > Support for directories, including recognition of ???_arch_foo.patch. From my rough estimation, only a few packages use '??_all_*.patch'. I haven't see any arch-dependent patches, and epatch deprecates that feature anyway. But I haven't checked the eclasses. Talking more about directories, there are two major patch formats used that way: git patches and debian patches. The latter are randomly named and have 'series' file which epatch does not handle. I'm not sure about how wide the support for directories needs to be done but generally even a glob will fit most of the current use cases: src_prepare() { local PATCHES=( "${WORKDIR}"/foo-patches/*.patch ) default } As for compression, the epatch uncompress support seems one of the biggest mistakes there. There's a lot of ebuilds using it, and most of them are using it *unnecessarily*. The ebuilds apply patch from $DISTDIR directly while PM unpacks it to $WORKDIR anyway... There are four semi-valid cases of applying compressed patches. Three of them stored compressed files in $FILESDIR which is a bit controversial. Fourth one uses some custom rpm2targz and unpack magic in src_unpack(); I don't think anyone would mind unpacking the patch by hand anyway. (In reply to comment #17) > From my rough estimation, only a few packages use '??_all_*.patch'. http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo/src/patchsets/ seems to indicate something else. (But it might depend on your definition of "few".) > I'm not sure about how wide the support for directories needs to be done but > generally even a glob will fit most of the current use cases: > > src_prepare() { > local PATCHES=( "${WORKDIR}"/foo-patches/*.patch ) > default > } Wasn't the starting point that PATCHES would spare an explicit phase function? It's sort of pointless if you then need src_prepare for globbing to work. > As for compression, the epatch uncompress support seems one of the biggest > mistakes there. There's a lot of ebuilds using it, and most of them are > using it *unnecessarily*. The ebuilds apply patch from $DISTDIR directly > while PM unpacks it to $WORKDIR anyway... I agree. Uncompressing should be done in src_unpack. > There are four semi-valid cases of applying compressed patches. Three of > them stored compressed files in $FILESDIR which is a bit controversial. That's not controversial, but a QA violation. Please file bugs for them. (In reply to comment #18) > (In reply to comment #17) > > From my rough estimation, only a few packages use '??_all_*.patch'. > > http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo/src/patchsets/ seems to > indicate something else. (But it might depend on your definition of "few".) I didn't even know we had that repo :). It's far much easier to grep that than unpack dozens of ebuilds. There's indeed a lot of ebuilds using 'all' there. But the sole consumer of arch-specific patches seems to be glibc nowadays. I've noticed them in a few ancient versions of gcc too. I believe that these two don't outweight the common case. That said, I believe that this thing is a bit mis-designed. If I were to guess what EPATCH_FORCE does, disabling enforcing specific patch naming would be far behind the last thing that came into my mind. If we really want that, it should be opt-in, not opt-out. > > I'm not sure about how wide the support for directories needs to be done but > > generally even a glob will fit most of the current use cases: > > > > src_prepare() { > > local PATCHES=( "${WORKDIR}"/foo-patches/*.patch ) > > default > > } > > Wasn't the starting point that PATCHES would spare an explicit phase > function? It's sort of pointless if you then need src_prepare for globbing > to work. Although I really don't mind the above syntax (since it's very clear in what it does and avoids polluting the env), I have to agree. Though I will feel a bit confused by things like: # is it a file? a directory? PATCHES=( "${WORKDIR}"/foo-patches ) What I'd really like to avoid is having 3-4 variables to control how the directory will be processed. Yet again, I would go for something that satisfies the common case -- like: applying all '.diff' and '.patch' files from the directory. No excludes, no suffix-changing and other unnecessary magic. If people really need stuff like that, they can use globs or specify files directly. They would have to do that for most of the conditional patches anyway. The goal of putting this in the package mangler isn't to handle every single obscure side-case that people can come up with. It's to handle the common stuff. So, what qualifies as common case then? IMHO, all of the following:
- Support for regular patch files
- Automatic -p* detection, overridable by explicit -p option
- Support for directories, with patches applied in lexical order of their
filenames
But I wonder if we couldn't follow a different approach: Have the default src_prepare call epatch if this function is defined, and otherwise fall back to a very basic patch function (defined by the PM).
"Automatic -p detection" is pretty nasty. Is it really so bad to strongly encourage everyone to use -p1? (In reply to comment #21) > But I wonder if we couldn't follow a different approach: Have the default > src_prepare call epatch if this function is defined, and otherwise fall back > to a very basic patch function (defined by the PM). Please don't. This sounds terribly fragile, to be honest. If someone wants to override the patching function, I believe that he can override the whole src_prepare() as well. (In reply to comment #22) > "Automatic -p detection" is pretty nasty. Is it really so bad to strongly > encourage everyone to use -p1? Hmm, maybe we could agree on using '-p1' in Gentoo repos (PATCHES array). Then we could just let portage optionally extend epatch_user since that's more of an optional feature. (In reply to comment #23) > > "Automatic -p detection" is pretty nasty. Is it really so bad to strongly > > encourage everyone to use -p1? One doesn't exclude the other. > Hmm, maybe we could agree on using '-p1' in Gentoo repos (PATCHES array). > Then we could just let portage optionally extend epatch_user since that's > more of an optional feature. Please don't, because then users would locally test their patches (with different -p depth), but the same patches would fail when used in the tree. yes, such a split up behavior sucks (In reply to comment #21) > So, what qualifies as common case then? IMHO, all of the following: > - Support for regular patch files > - Automatic -p* detection, overridable by explicit -p option > - Support for directories, with patches applied in lexical order of their > filenames Ok, assuming that the fore-mentioned points are acceptable. 1) do we want to name this 'epatch' and deprecate the eclass-defined epatch? or how to name it? 2) how exactly should -p* detection work? the current way of 'dry-run' is probably the least fragile yet not really output-friendly. It would be great if we could separately determine patch-level and then apply the patch, potentially failing due to content mis-match. Do we support the case when -p* levels lower than the correct one match existing files? Consider the following: - patch names 'a/src/foo.c', - we have 'src/foo.c' and 'foo.c', - patch applies correctly applies to the latter one (i.e. with -p2). That's IMO the most fragile thing about -p guessing -- you can't be sure which one is correct. If the patch becomes outdated, it may mistakenly get applied to the wrong file (very unlikely but still). As an alternative, we could limit the -p detection to the first patchlevel which matches an existing file. In this case, the fore-mentioned patch will stop at -p1 seeing 'src/foo.c'. It will limit the use a bit but make the behavior a little less fragile. 3) honestly, I'd like to see more git patch support. Potentially starting -p guessing with -p1 would be helpful. Applying git renames, modesetting would be awesome. (In reply to comment #26) > 1) do we want to name this 'epatch' and deprecate the eclass-defined epatch? You've answered this yourself in comment #0: "[...] we may want to use a different name and implement only a sane subset of it." As it looks like we won't go for a drop-in replacement, we should keep the eclass function for people who need its full functionality. And thinking about it again, if we keep the full-fledged epatch in the eclass, then I won't cling to -p* autodetection in the PM implemented version. (Directory support is essential though.) > or how to name it? Good question. Ask in gentoo-dev how the bikeshed should be painted. ;-) (In reply to comment #19) > Yet again, I would go for something that satisfies the common case -- like: > applying all '.diff' and '.patch' files from the directory. That's the way to go. Not "all files" (as you've worded it in your message to gentoo-dev), but only *.patch and *.diff, for two reasons: - It's very common for patchsets to include a README file. - You want to exclude any ".*", "*~", or "*.bak", especially for the user patch feature. (In reply to comment #28) > - You want to exclude any ".*", "*~", or "*.bak", especially for the user > patch > feature. Yes to .*, no to the rest. Otherwise we'll just be opening ourselves up to "my editor calls backup files .old so you should exclude that too". (In reply to comment #28) > (In reply to comment #19) > > Yet again, I would go for something that satisfies the common case -- like: > > applying all '.diff' and '.patch' files from the directory. > > That's the way to go. Not "all files" (as you've worded it in your message > to gentoo-dev), but only *.patch and *.diff, for two reasons: > - It's very common for patchsets to include a README file. > - You want to exclude any ".*", "*~", or "*.bak", especially for the user > patch > feature. I agree with you, that's just misunderstanding between us. There's also the Debian 'series' file but I doubt it'd be catched. (In reply to comment #29) > (In reply to comment #28) > > - You want to exclude any ".*", "*~", or "*.bak", especially for the user > > patch > > feature. > > Yes to .*, no to the rest. Otherwise we'll just be opening ourselves up to > "my editor calls backup files .old so you should exclude that too". Erm, '*.diff' and '*.patch' opt-in excludes everything else. *** Bug 469664 has been marked as a duplicate of this bug. *** To summarise again what has been discussed so far, the patch applying function should support the following features:
- regular patch files
- directories, with patches applied in lexical order of their filenames,
only files named *.diff and *.patch
- still to be decided if either
a) automatic -p* detection, overridable by explicit -p option, or
b) only -p1
Furthermore, we don't have a name for the new function yet.
I propose a new ebuild stage src_patch after src_unpack but before src_prepare.
Right after src_patch is executed, for every element of PATCHES and $PORTAGE_CONFIGROOT/etc/portage/patches/$CATEGORY/{$PN,$P,$PF}:
- A relative path is relative to $FILESDIR
- If it's a file, it's applied.
- If it's a directory, all patches in it and its subdirectories
are applied in order as if their filenames were `sort`-ed.
The default is -p1. A patch's filename may be suffixed by a number. In that case it's used for -p. file.patch42 would mean `patch -p42 < $FILESDIR/file.patch42`
A file is considered a patch if its name is of the form '.*(diff|patch)[0-9]*'.
I don't think compressed patches should be supported. Maybe a new eclass function could uncompress a patch and append it to PATCHES.
In case patches are downloaded the directory where they are or individual filenames can be put in PATCHES.
In src_patch files can be modified in other ways (e.g. sed) and elements can be appended to PATCHES depending on some logic (USE flags, ABI, broken versions of gcc).
(In reply to Hristo Venev from comment #33) > I propose a new ebuild stage src_patch after src_unpack but before > src_prepare. It has to go _after_ the patches normally applied by src_prepare, since otherwise the user patches may introduce conflicts that prevent the other patches from applying. For reference, see apply_user_patches which was dropped from EAPI 5: http://thread.gmane.org/gmane.linux.gentoo.pms/516/focus=520 No patches should be done in src_prepare. Some user patches may modify files used by src_prepare so user patching should be done before src_prepare. User patches may introduce conflicts so in-tree patching should be done before that: src_unpack src_patch apply in-tree patches apply user patches src_prepare ... It does break backwards compatibility but that's what EAPI's are about, right? (In reply to Hristo Venev from comment #33) > I propose a new ebuild stage src_patch after src_unpack but before > src_prepare. What is the new role of src_prepare() then? > Right after src_patch is executed, for every element of PATCHES and > $PORTAGE_CONFIGROOT/etc/portage/patches/$CATEGORY/{$PN,$P,$PF}: > - A relative path is relative to $FILESDIR This is confusing. Suddenly relative paths in PATCHES mean different than relative paths within the patch. > - If it's a file, it's applied. > - If it's a directory, all patches in it and its subdirectories > are applied in order as if their filenames were `sort`-ed. > > The default is -p1. A patch's filename may be suffixed by a number. In that > case it's used for -p. file.patch42 would mean `patch -p42 < > $FILESDIR/file.patch42` No. This is completely custom and it's going to be a mess having all those numbers running around. |