Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PCRE2 #2138

Merged
merged 1 commit into from Jan 22, 2022
Merged

PCRE2 #2138

merged 1 commit into from Jan 22, 2022

Conversation

jschueller
Copy link
Contributor

Closes #2120

Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these changes! FWIW everything here looks good to me, but I think you also need to update the package being installed in Tools/CI-linux-install.sh and maybe Tools/mkwindows.sh, if it's still used.

@wsfulton
Copy link
Member

wsfulton commented Jan 5, 2022

Did you see the Github CI failures related to this change, such as:

SWIG:1: Error: PCRE capture replacement failed while matching "wx(?!EVT)(.*)" using "m_width" - request for group 1 is greater than the number of captures 0.
SWIG:1: Error: PCRE compilation failed: 'unmatched closing parenthesis' in '(.*?)(nsa)(.*?)\2(.*?)\2(.*?)\2(.*)':1.

SWIG:32: Error: Matching "Foo" against regex "[A-Z]\w*[A-Z]" failed: -51

Source/Swig/misc.c Outdated Show resolved Hide resolved
@jschueller jschueller force-pushed the pcre2 branch 18 times, most recently from d73cdc6 to e04f11e Compare January 8, 2022 09:26
@wsfulton
Copy link
Member

wsfulton commented Jan 8, 2022

@jschueller thanks for taking on this change. Now that Travis tests are working, and it looks like you are figuring out the Appveyor changes, can you stop force pushing. In theory pushes to this branch changing just appveyor.yml should not (unnecessarily) do the very compute intensive GHA build because of:

on:
push:
paths-ignore:
- 'CHANGES*'
- 'Doc/**'
- 'appveyor.yml'
.
This is not working, but I think it is because you are force pushing. If you just add additional commits to just appveyor.yml, just the Appveyor CI should run. In my experience, it can take a lot of commits before configuring appveyor.yml correctly. You could also do this in your own github repo if you don't want us to see every small attempt at getting it right and then push the final working version here. If you need help with Appveyor, I can add it to my queue of work.

@wsfulton
Copy link
Member

wsfulton commented Jan 8, 2022

I tested the distribution build with these changes and they work okay and the windows build seems to work with pcre2-10.39.tar.bz2.

@jschueller
Copy link
Contributor Author

yes I wasnt too successful, seems like I have to build pcre from source but didtn manage to build something compatible
you can have a go

appveyor-retry appveyor DownloadFile https://github.com/PhilipHazel/pcre2/archive/refs/tags/pcre2-10.39.zip
7z x pcre2-10.39.zip

@wsfulton
Copy link
Member

wsfulton commented Jan 8, 2022

yes I wasnt too successful, seems like I have to build pcre from source but didtn manage to build something compatible
you can have a go

Am confused! I successfully got a Windows build. What do you want me to have a go at? BTW, I also checked that your changes to pcre-build.sh works on an Ubuntu box (this script builds pcre2 from source, run Tools/pcre-build.sh --help for more info).

@jschueller
Copy link
Contributor Author

I had pcre built with msvc (with cmake) but then it failed to link when used in swig (a cryptic message about crt libs),
maybe you know better which tools/options must be used

@wsfulton
Copy link
Member

wsfulton commented Jan 9, 2022

I see. CMake is a secondary build system and not mandatory to keep working. Best left to those who rely on it to fix it.

@jschueller
Copy link
Contributor Author

swig's cmake is ok, I just couldnt build an usable pcre on appveyor

@wsfulton
Copy link
Member

wsfulton commented Jan 9, 2022

I see, but obtaining and using the dependencies are just as important as building SWIG itself. The instructions in Windows.html are meant to be a set of simple get going instructions including obtaining and using PCRE. Maybe those will help inspire what you need to do for PCRE2.

@wsfulton wsfulton added this to the swig-4.1 milestone Jan 11, 2022
@jschueller
Copy link
Contributor Author

@wsfulton I think I got it right, the only remaining appveyor failures look like in master

@wsfulton
Copy link
Member

Looking good. I've put in a fix for mingw on master. I missed it going wrong on 1 Jan when the mingw folk re-organised the their packages. Could you please fix the consequent conflict and push again, then hopefully appveyor testing for the pull request will go green. Note that cygwin Appveyor testing on master are marked as allow_failures, I don't know what is wrong there.

@wsfulton
Copy link
Member

All tests passed. Yay! The only thing that really needs doing now is to patch up Windows.html to reflect the kind of changes you have put into Appveyor.yml. It's real shame that nuget does not provide a pcre2 package like it does pcre, otherwise the docs would be much easier to update.

@wsfulton
Copy link
Member

One last thing, we have Tools/CMake/FindPCRE.cmake and a new file FindPCRE2.cmake. Could you please remove FindPCRE.cmake and move FindPCRE2 into Tools/CMake. Tools/nuget-install.cmd can also be deleted now given since nuget is no longer used and you used appveyor-retry to do the retries.

@jschueller
Copy link
Contributor Author

ok, done

@wsfulton wsfulton merged commit 15515f3 into swig:master Jan 22, 2022
@jschueller jschueller deleted the pcre2 branch January 23, 2022 15:55
espindula pushed a commit to espindula/br-blfs that referenced this pull request Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SWIG depends on obsolete pcre library
4 participants