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.

Sign up for GitHub

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

Jump to bottom

Update PCRE2 version to 10.34. Fixes #35322 #35459 #35607

Merged
merged 4 commits into from
May 1, 2020
Merged

Update PCRE2 version to 10.34. Fixes #35322 #35459 #35607

merged 4 commits into from
May 1, 2020

Conversation

Micket
Copy link
Contributor

@Micket Micket commented Apr 27, 2020 •
edited by fredrikekre
Loading

@Micket Micket mentioned this pull request Apr 27, 2020
Closed
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 28, 2020

Good to know there's a new release with fixes. What's the status of the equivalent PR to Yggdrasil?

@Micket
Copy link
Contributor Author

Micket commented Apr 28, 2020

@vtjnash They already bumped the version string to 10.34
https://github.com/JuliaPackaging/Yggdrasil/blob/master/P/PCRE2/build_tarballs.jl#L6
they just haven't build a tarball for release as far as I can tell. I don't know how this packaging thing works.

@Micket Micket changed the title Update PCRE2 version to 10.43. Fixes #35322 #35459 Update PCRE2 version to 10.34. Fixes #35322 #35459 Apr 28, 2020
@KristofferC
Copy link
Sponsor Member

they just haven't build a tarball for release as far as I can tell. I

From what I can see, the release with the tarballs has been made (I think it happens automatically when merging a PR to Yggdrasil):

https://github.com/JuliaBinaryWrappers/PCRE2_jll.jl/releases

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 28, 2020

Thanks. So we just need to run ./contrib/refresh_bb_tarballs.sh pcre on this?

@KristofferC
Copy link
Sponsor Member

KristofferC commented Apr 28, 2020 •
edited
Loading

I think this URL needs to be tweaked to point to the JuliaBinaryWrappers/PCRE2_jll.jl repo, instead of the yggdrasil one?

julia/deps/pcre.mk

Line 58 in 5cb3f3f

PCRE_BB_URL_BASE := https://github.com/JuliaPackaging/Yggdrasil/releases/download/PCRE2-v$(PCRE_VER)-$(PCRE_BB_REL)

@Micket
Copy link
Contributor Author

Micket commented Apr 28, 2020

@KristofferC I suspect this one is also wrong then?

julia/deps/libuv.mk

Line 67 in 04234fb

LIBUV_BB_URL_BASE := https://github.com/JuliaPackaging/Yggdrasil/releases/download/LibUV-v2+$(LIBUV_VER)-julia+$(LIBUV_BB_REL)

But these ones should still use Yggdrasil?

julia/deps/dsfmt.mk

Line 68 in c9786e6

DSFMT_BB_URL_BASE := https://github.com/JuliaPackaging/Yggdrasil/releases/download/dSFMT-v$(DSFMT_VER)-$(DSFMT_BB_REL)

julia/deps/unwind.mk

Line 107 in e6dd72f

UNWIND_BB_URL_BASE := https://github.com/JuliaPackaging/Yggdrasil/releases/download/LibUnwind-v$(UNWIND_VER)+$(UNWIND_BB_REL)

Anyway, I'll update this PR with the correct url for pcre (when I get back home)

@KristofferC
Copy link
Sponsor Member

I suspect this one is also wrong then?

I am not sure. It might be that only some of these have transitioned to the JuliaBinaryWrappers organization.

@ViralBShah
Copy link
Member

ViralBShah commented Apr 28, 2020 •
edited
Loading

Yes, we stopped publishing JLLs to Yggdrasil releases - except perhaps tollchains. The old releases may be there, but the new ones are likely to be in JuliaBinaryWrappers.

https://github.com/JuliaPackaging/Yggdrasil/releases/download

@giordano may know the details better.

@giordano
Copy link
Contributor

Yes, until ~August last year the tarballs were published under Yggdrasil, but then we started using the more well organised JLL packages which are under the JuliaBinaryWrappers organisation. In the Julia build system only when necessary a newer version of a tarball is used and all the new ones (i.e., after ~August) will be found under JuliaBinaryWrappers.

KristofferC
KristofferC approved these changes Apr 30, 2020
View reviewed changes
Copy link
Sponsor Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

LGTM but would be good with a quick look from @staticfloat as well.

@KristofferC
Copy link
Sponsor Member

Actually, the old checksum files should maybe be removed?

KristofferC
KristofferC reviewed Apr 30, 2020
View reviewed changes
deps/pcre.mk
@@ -55,7 +55,7 @@ fastcheck-pcre: check-pcre
check-pcre: $(BUILDDIR)/pcre2-$(PCRE_VER)/build-checked

else # USE_BINARYBUILDER_PCRE
PCRE_BB_URL_BASE := https://github.com/JuliaPackaging/Yggdrasil/releases/download/PCRE2-v$(PCRE_VER)-$(PCRE_BB_REL)
PCRE_BB_URL_BASE := https://github.com/JuliaBinaryWrappers/PCRE2_jll.jl/releases/download/PCRE2-v$(PCRE_VER).0+$(PCRE_BB_REL)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Should this .0 be moved to PCRE_VER in Make.inc instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it wouldn't work when downloading the sources instead:
https://github.com/JuliaLang/julia/blob/master/deps/pcre.mk#L9
(I'm not sure why PCRE2_jll insist on adding another number suffix, it really is just 10.34, https://ftp.pcre.org/pub/pcre/)

Copy link
Contributor

Choose a reason for hiding this comment

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

(I'm not sure why PCRE2_jll insist on adding another number suffix, it really is just 10.34, ftp.pcre.org/pub/pcre)

Because JLL packages use Julia's VersionNumber representation in string interpolation:

julia> "$(v"10.34")"
"10.34.0"

@staticfloat
Copy link
Sponsor Member

Looks good to me, but yes, please delete the old PCRE2 checksums as well.

@fredrikekre fredrikekre merged commit fbab705 into JuliaLang:master May 1, 2020
@omus omus mentioned this pull request May 4, 2020
Closed
Keno added a commit that referenced this pull request May 5, 2020
@Keno
Revert "Update PCRE2 version to 10.34, fixes #35322, fixes #35459. (#…
dc8ebd6 
…35607)"

This reverts commit fbab705.
@Keno Keno mentioned this pull request May 5, 2020
Merged
@Keno
Copy link
Member

Keno commented May 5, 2020

We've been seeing intermittent PCRE errors after this. I'm gonna revert this for 1.5 and we can re-apply and investigate for 1.6.

JeffBezanson pushed a commit that referenced this pull request May 5, 2020
@Keno
Revert "Update PCRE2 version to 10.34, fixes #35322, fixes #35459. (#…
84ae4a4 
…35607)" (#35750)

This reverts commit fbab705.
@StefanKarpinski
Copy link
Sponsor Member

It's possible we can unrevert this after #35773.

@BenjaminGalliot
Copy link
Contributor

@Keno, do people still have this OS X issue?

StefanKarpinski added a commit that referenced this pull request Sep 22, 2020
@StefanKarpinski
Update PCRE2 version to 10.34, fixes #35322, fixes #35459.
2a255e8 
Restore #35607
Revert #35750

This reverts commit 84ae4a4.
@StefanKarpinski StefanKarpinski mentioned this pull request Sep 22, 2020
Merged
StefanKarpinski added a commit that referenced this pull request Sep 23, 2020
@StefanKarpinski
Update PCRE2 version to 10.34, fixes #35322, fixes #35459. (#37688)
9b39974 
Restore #35607
Revert #35750

This reverts commit 84ae4a4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giordano giordano giordano left review comments

@KristofferC KristofferC KristofferC approved these changes

@staticfloat staticfloat Awaiting requested review from staticfloat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unicode named groups for regular expressions Avoidable "PCRE compilation error" by allowing more than ASCII
10 participants
@Micket @vtjnash @KristofferC @ViralBShah @giordano @staticfloat @Keno @StefanKarpinski @BenjaminGalliot @fredrikekre