zsh: use libpcre2 instead of libpcre #22197
Conversation
There was a problem hiding this comment.
In one your single commit, you are trying to split so many changes, which are not described anywhere and who knows why they are required and why do we need them.
Many things are really unrelated to use libpcre2 instead of pcre, which you tried to describe that in commit description. Please try to avoid that for the next time.
What I can suggest to you is to
- Split your commit into multiple ones (no need this time as you should stick only to changes related to libpcre2)
- Other changes should have separate patches as it is done in upstream repository.
You can do that e.g. on GitHub when you go to each commit or PR and ammend to the end of the URL suffix .patch and that should be done here. Example: zsh-users/zsh@b62e911
| @@ -20,6 +20,7 @@ PKG_LICENSE:=ZSH | |||
| PKG_LICENSE_FILES:=LICENCE | |||
| PKG_CPE_ID:=cpe:/a:zsh_project:zsh | |||
|
|
|||
| PKG_FIXUP:=autoreconf | |||
There was a problem hiding this comment.
Why this is required now? In the commit description, there is no such info on this one.
There was a problem hiding this comment.
from commit:
use autoconf: git version no longer supports traditional configure script, so it was easier to patch configure.ac
Provided patch is more or less a back port/cherry pick. As zsh's master no longer ships with configure script, instead uses auto-tools, there would had been a huge task of patching configure script that would also had been a huge task for reviewers to check what's changed. Zsh has supported auto-tools for quite some time.
from docs:
Packages that are using auto-tools should work with simply “PKG_FIXUP:=autoreconf”.
Why this is required now? In the commit description, there is no such info on this one.
This is required to use auto-tools over traditional configure script as said in documents.
There was a problem hiding this comment.
Ok, thanks! Would it be possible to keep this in separate commit to have it clear and simple? This from my point should not be in one huge patch, which would be really hard to track changes.
There was a problem hiding this comment.
But we are not using master branch and we are using still the same version. Dont you forget to add something?
There was a problem hiding this comment.
To succeed, we are swapping from traditional configure to configure.ac which makes build process different (equal to master branch minus some bells and whistles not available on release version). It's been like this in autotools version for quite some time, but we haven't utilised it until now.
I could apply another huge patch to fix traditional configure, that would be other way around this, but wouldn't that make things even more complicated? I attempted it in the first place and found out pretty soon that changes required in traditional configure script would be massive and definitely break if it ever gets updated due to amount of changes. So, I sticked to with current autotools version, traditional isn't even supported anymore; whole script has been removed from master branch. Probably been outdated long ago.
|
There is reason for every change that this patch makes. When next release of zsh comes out, patch will be incompatible and all changes it makes, are merged to that release, except LN_S stuff, which is more a problem with openwrt build system. Hopefully though this patch will be then obsolete on that as well and AC_PROG_LN is included in auto-tools macros. Nevertheless, with this you get what you requested for zsh; libpcre2 instead of libpcre. I am happy with that as well, as I use zsh personally, and libpcre in my case was depend only for zsh; now I no longer have to install it at all. Please, clarify if you have proposals for changes. Most of what I wrote here/explained was already in the commit, but pretty shortly. Do you want me to expand descriptions of changes? What about patch or Makefile? Do you have proposals for changes? For the moment, I am not sure if you want or how you would want me to change anything. Maybe separate libpcre2 and auto-tools fix to different patches? |
oskarirauta
force-pushed
the
zsh
branch
2 times, most recently
from
b03626b to
807bcb1
Compare
|
Patch splitted to 2 patches. Commit message expanded. |
|
Uhh, you wrote a lot. I need to go through all of it. I will not make it
today. Give me day or two. I am afraid that tomorrow my calendar is nearly
full and I am spending the day outside of the computer.
Dne ne 24. 9. 2023 19:42 uživatel Oskari Rauta ***@***.***>
napsal:
… @oskarirauta <https://github.com/oskarirauta> requested your review on:
#22197 <#22197> zsh: use libpcre2
instead of libpcre.
—
Reply to this email directly, view it on GitHub
<#22197 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7IDVG4VZ5O4OXVGQG63ATX4BWI7ANCNFSM6AAAAAA5END3KY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
Just answered to all the questions as explanatory as possible :) I have possibility to work with my laptop through a vpn connection, but it's way better locally. Especially when I don't know if I have short days, or do they go past 16 hours.. If I am not answering briefly, then I just don't have time; bare patiently, and I'll get back on topic as soon as I am able to. But surely, if there's not much changes necessary, I probably can use my breaks to do some work; Only change that I think might be in place, is patch to that test file.. I just patched that as well, since it was relevant even when it's unnecessary for result. |
|
You should also review this.. |
|
Well, I looked over the patch (unfortunately, I don't have enough spare time at the moment for full careful analysis). But unfortunately, at the moment, I dont't even have a device to test it on (that patched version works fine). I'll try to at least check that it builds correctly at next holidays. |
|
I am running with it. Builds and runs. Good luck. |
|
Have you had a chance yet to check this? |
utils/zsh/patches/020-replace-missing-autotools-ln-with-ln_s.patch
Outdated
Show resolved
Hide resolved
Yes, I bet that there is a reason for that, but it should be described in the patch itself that people will know, why it was required. Hiding it (not exactly, but you got the point) in multiple changes in one huge commit is not good idea.
Yeah, I do not have installed libpcre anymore either!
One option is to move it from commit description to patch's commit description or have it on both places. About patches - because it is a really huge diff, I havent look at it so far. Quite busy with other things. |
Yes, but make a concrete proposal with example that I can apply from here. That way you get it just the way you want, because you already know what this does and so on, so you know what you want there to read; instead of me guessing what that might be.
Depends where you look at it from; smaller than some of my private kernel patches.... I am also quite busy, and soon this branch get's old enough to have conflicts with repo and I absolutely don't have more time on my timeline for this. One solution would be to switch to current git source and only use my autotools patch, but I know what is the opinion on using trunk versions. Why we need them? Simple. It is on newer version than current release version being used. There haven't yet released a version that has this. And autotools patch is just to fix missing macro. |
oskarirauta
force-pushed
the
zsh
branch
3 times, most recently
from
762403e to
ce93a22
Compare
|
Patches updated with commentary and signed off. |
|
actually, patches aren't yet correct git-formatted patches, but it is no my business, if any, so it's on @BKPepe to review that part :-D |
|
@msva looks like it's a no go. |
|
@oskarirauta you are welcome my guy https://gist.github.com/Ansuel/5f0901837fc65c01025f2c62002166a3 (patch are to refresh but they are not with correct formal) the tar they generate include some files that are ignored by gitingore... very stupid thing honestly and they should really fix this... Also the second patch... should be reported in openwrt main repo as it seems ""easy"" to fix... @mpratt14 maybe can help? (autotools missing macro @ln@) |
|
in the autotools patches called "relocatable" we end up hardcoding the paths anyway... would it be better to just have LN_S exported from rules.mk? |
|
@mpratt14 from what i can see the problem is that the aclocal.m4 is not getting loaded correctly... AC_PROG_LN is a non standard macro and is present in aclocal.m4 but is not getting evaluated??? |
|
@mpratt14 this is the zsh aclocal.m4 https://github.com/zsh-users/zsh/blob/master/aclocal.m4 |
|
maybe, I'm bad with the m4 files so I wouldn't know without really digging deep I just happened to notice a coincidence that in rules.mk we define LN but not LN_S and it's also not exported...its a simple enough definition so I don't know a reason we shouldn't just export it |
|
Nope adding export LN on rules.mk doesn't fix it |
|
just a guess, maybe the aclocal.m4 should be renamed because it's getting deleted before autoreconf |
yes that is exactly the problem... aclocal is getting wiped... i see normally acinclude should be used... but renaming it doesn't fix it... now i'm checking makefile.in |
|
I'm getting so triggered by these stupid thing.... |
|
add a hook to Hooks/Prepare/Post instead of patching that rename maybe the makefile is listing aclocal.m4 in a list of local m4 files and that needs a patch? |
|
@mpratt14 very fun indeed... for some reason renaming and patching the file is not liked by zsh build system... But i found the real cause... Yes we were right from the start but it's not autoreconf that deleted the file... it's this bad boy https://github.com/openwrt/openwrt/blob/main/include/autotools.mk#L81 that by default remove aclocal.m4... problem is that zsh used the default name to include custom macro and this is getting removed... so zsh fault and openwrt deleting files... by setting the variable empty everything compile correctly as the macro are correctly referenced... |
|
@BKPepe took care of improving this... For me this is ready. |
|
@Ansuel I hate to be this picky, but there's a typo in your makefile comment |
|
I’m fine with all. Can’t review without problems now with Mobile device, but my work was pcre2 modification, autotools was just a necessary bad, and I’m glad you got it work out even better.Michael Pratt ***@***.***> kirjoitti 5.10.2023 kello 6.03:
@Ansuel I hate to be this picky, but there's a typo in your makefile comment
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@mpratt14 np but where? aclocak ? I updated, can you recheck. @oskarirauta ok @BKPepe for me it's ok to merge. |
In preparation to PCRE2 fixup, use autoreconf PKG_FIXUP as a better configure system instead of configure script. This is needed to reduce upcoming patch to migrate to PCRE2 library. To correctly use autoreconf it's needed to declare empty PKG_REMOVE_FILES. zsh include custom macro in the default aclocal.m4 When autoreconf PKG_FIXUP is used, if PKG_REMOVE_FILES is not defined, it's set to remove the file aclocal.m4 by default resulting in problem with the custom macro AC_PROG_LN. Signed-off-by: Oskari Rauta <[email protected]> [ split to 2 commit, add PKG_REMOVE_FILES, reword commit description ] Signed-off-by: Christian Marangi <[email protected]>
|
@Ansuel I finally got a chance to check. LGTM. |
yeah that |
| @@ -0,0 +1,648 @@ | |||
| From fabdb4164288b21f1323f783d257212e5aacecd4 Mon Sep 17 00:00:00 2001 | |||
There was a problem hiding this comment.
Regarding this patch, I think we can do it even more better.
I am not sure, why it is labeled as hack, when it seems by quick looking at this patch that it contains actually at least four commits from upstream, so we can do it as patch series as this is rather backport than hack.
Hack in my opinion is something, which is not included in upstream and won't be.
- https://github.com/zsh-users/zsh/commit/b62e911341c8ec7446378b477c47da4256053dc0.patch
- https://github.com/zsh-users/zsh/commit/f3f371deb376478176866fd770fbcf9bc0d0609f.patch
- https://github.com/zsh-users/zsh/commit/b4d1c756f50909b4a13e5c8fe5f26f71e9d54f63.patch
- https://github.com/zsh-users/zsh/commit/10bdbd8b5b0b43445aff23dcd412f25cf6aa328a.patch
Yeah, we need to remove the things, which we need, but still it will look better. :-)
Btw. Yes, I need to admit that this is a big improvement than it was, but it could be even better. 💪
There was a problem hiding this comment.
@BKPepe indeed... that was my original idea and was blocked by the pkg fixup... Took care of backporting the patch and now it's much cleaner and direct... just 5 backport patch not touched (aside from the changelog commented out)
There was a problem hiding this comment.
See @oskarirauta you should learn from this guy. :) 👏 👏 Awesome work as always.
Backport PCRE2 patches from upstream and move package to PCRE2 library as PCRE is EOL and won't receive any security update anymore. Patch are backported with minimal change, only the Changelog change is commented out as it would conflict and makes no sense to adapt for the purpose of backport patches. Signed-off-by: Christian Marangi <[email protected]>
|
CI passed, thank you @Ansuel, let's give it time until evening, so someone can go through it. If no objections, otherwise I will merge this. |
|
by the way, someone should inform the zsh developers that having a macro file in-project named "aclocal.m4" is deprecated and they should rename it or have an "m4" directory. |
|
someone should tell openwrt people that zsh developers value backwards compatibility over changing file naming |
|
bold to say this to the openwrt project itself where the base is to support old and deprecated devices... also 2012... https://lists.gnu.org/archive/html/automake-patches/2012-12/msg00079.html |
|
one can have both backwards compatibility AND the new method, we don't have to choose one... would a symlink named aclocal.m4 targeting a different named macro file work? |
Changes:
Maintainer: @msva
Compile tested: x86_64, latest git
Run tested: x86_64, latest git
Description:
zsh is my favourite shell. I found issue #22006 which mentioned zsh and that's where idea originated.
Patch can be removed when official zsh is next time updated (might take quite some time)