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

perl 5.36.0 #112161

Closed
wants to merge 42 commits into from
Closed

perl 5.36.0 #112161

wants to merge 42 commits into from

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Oct 2, 2022

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Starting over on #102549 since some changes have gone in other PRs like #108663.

Planning to cherry-pick the commits from other PR once we have readout of linkage failures.

@cho-m cho-m added do not merge CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Oct 2, 2022
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Oct 2, 2022
@cho-m cho-m added the CI-linux-self-hosted Build on Linux self-hosted runner label Oct 2, 2022
@danielnachun
Copy link
Contributor

Perl 5.37 is out now, so we should try that instead.

@danielnachun
Copy link
Contributor

Also from prior testing, the self-hosted runner is not needed just for dependent testing, so you might be able to run this faster if you remove that flag.

@cho-m
Copy link
Member Author

cho-m commented Oct 2, 2022

Perl 5.37 is out now, so we should try that instead.

Odd numbers are development versions. Latest stable is still 5.36.

@cho-m cho-m removed the CI-linux-self-hosted Build on Linux self-hosted runner label Oct 2, 2022
@cho-m cho-m force-pushed the bump-perl-5.36.0-alt branch from 012a086 to fdcb455 Compare October 2, 2022 21:05
Formula/perl.rb Show resolved Hide resolved
@danielnachun
Copy link
Contributor

Perl 5.37 is out now, so we should try that instead.

Odd numbers are development versions. Latest stable is still 5.36.

Ah I did not know that. Should we add a comment explaining that?

@cho-m cho-m force-pushed the bump-perl-5.36.0-alt branch from fdcb455 to 41623be Compare October 3, 2022 01:26
@cho-m cho-m added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-linux-self-hosted Build on Linux self-hosted runner long build Set a long timeout for formula testing no long build conflict Do not allow merging other pull requests when files conflict with this one and removed do not merge labels Oct 3, 2022
man1.install libexec/"man/man1/biber.1"
(pkgshare/"test").install "t/tdata/annotations.bcf", "t/tdata/annotations.bib"
Copy link
Member Author

Choose a reason for hiding this comment

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

Just picked the first test from directory. Not sure if there is a better one. We've been hit a few times by upstream modifying the test.bcf file with new releases. Unless we save a copy, it didn't seem worth seeing CI failures related to checksum mismatches or version incompatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize there was test directory but that seems like a much more reliable solution.

@danielnachun
Copy link
Contributor

Linux-specific failures:

  • subversion: top-level mandir
  • feedgnuplot: top-level mandir
  • grokj2k: we're on a very old release - the newest release builds fine on Linux
  • pidgin: needs a fix for XML::Parser - it should use ENV.prepend_path instead of ENV.prepend when adding the library path of intltool to PERL5LIB.

@cho-m
Copy link
Member Author

cho-m commented Oct 3, 2022

  • grokj2k: we're on a very old release - the newest release builds fine on Linux

I think we've had issues on macOS when trying to update grokj2k. Right after I fixed formula in #95683 (9.2.0 --> 9.7.1), upstream broke build in next patch #96168 and we haven't been able to update since.

Not sure about current version, but it was one of failures in previous perl PR #102549 so I want to avoid blocking perl update on this.


  • pidgin: needs a fix for XML::Parser - it should use ENV.prepend_path instead of ENV.prepend when adding the library path of intltool to PERL5LIB.

I was previously wondering if we should enable perl vendor directory and create a perl-xml-parser formula just to avoid all these workarounds due to common intltool.m4 script.

Though, most formulae are moving away from intltool so probably not important.

@fxcoudert fxcoudert removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Oct 3, 2022
@miles170 miles170 mentioned this pull request Oct 3, 2022
6 tasks
@cho-m
Copy link
Member Author

cho-m commented Oct 3, 2022

Looks like abricate is failing everywhere. Given we don't have a Monterey bottle, it was probably broken since. I might try switching to macOS Perl as the Perl dependencies are pre-installed (may not be possible depending on bioperl). Relying on cpanminus may have made the build harder to reproduce.

man-db failed in previous PR too. Will need to determine if it needs a revision bump. Test failure output isn't very useful.

@cho-m cho-m mentioned this pull request Oct 3, 2022
6 tasks
@cho-m
Copy link
Member Author

cho-m commented Oct 3, 2022

pidgin is actually due to intltool failure (XML::Parser is a compiled library)

$ PERL5LIB=/home/linuxbrew/.linuxbrew/opt/intltool/libexec/lib/perl5 perl -e "require XML::Parser"
Expat.c: loadable library and perl binaries are mismatched (got first handshake key 0xeb00080, needed 0xeb80080)

$ file /home/linuxbrew/.linuxbrew/opt/intltool/libexec/lib/perl5/x86_64-linux-thread-multi/auto/XML/Parser/Expat/Expat.so
/home/linuxbrew/.linuxbrew/opt/intltool/libexec/lib/perl5/x86_64-linux-thread-multi/auto/XML/Parser/Expat/Expat.so: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, BuildID[sha1]=ad3c15e5bc2cbc090fb4422af4e1e72ab735677e, not stripped

bayandin and others added 5 commits October 3, 2022 11:08
Co-authored-by: Michael Cho <cho-m@tuta.io>
Co-authored-by: Michael Cho <cho-m@tuta.io>
Co-authored-by: Michael Cho <cho-m@tuta.io>
@cho-m cho-m added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Oct 3, 2022
@cho-m cho-m force-pushed the bump-perl-5.36.0-alt branch from 41623be to 1a059aa Compare October 3, 2022 18:27
Comment on lines -226 to +233
system "make", "swig-pl"
system "make", "install-swig-pl"
system "make", "swig-pl-lib"
system "make", "install-swig-pl-lib"
cd "subversion/bindings/swig/perl/native" do
system perl, "Makefile.PL", "PREFIX=#{prefix}", "INSTALLSITEMAN3DIR=#{man3}"
system "make", "install"
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively could have

  • manually moved files (man3.install ...) or
  • modified the Makefile
    inreplace "Makefile", "$(PERL) Makefile.PL PREFIX=$(prefix)", "\\0 INSTALLSITEMAN3DIR=#{man3}"

The current approach is officially documented method for adding options during Perl build https://github.com/apache/subversion/blob/1.14.2/subversion/bindings/swig/INSTALL#L217-L236

PREFIX is from the Makefile https://github.com/apache/subversion/blob/1.14.2/Makefile.in#L883-L884 while INSTALLSITEMAN3DIR is to fix the manpage installation.

Copy link
Contributor

Choose a reason for hiding this comment

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

The documented approach looks pretty clean so it seems fine to me.

@@ -5,7 +5,7 @@ class Grokj2k < Formula
url "https://github.com/GrokImageCompression/grok/archive/v9.7.1.tar.gz"
sha256 "a7d433dca92b035349ef8203eb44cb6d0b2c9b41aecd2d12872a9ca2761e0606"
license "AGPL-3.0-or-later"
revision 1
revision 2
Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully, only remaining failure will be Linux grokj2k. I don't think we can fix in this PR so may need to follow up afterward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we should follow up with this in a PR to upgrade grokj2k.

Copy link
Member Author

@cho-m cho-m Oct 3, 2022

Choose a reason for hiding this comment

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

Working on it in #112269. Current problem is macOS Perl headers conflict with C++ headers. It builds for me if I disable exiftool feature. Need to look into it a bit more:

/Library/Developer/CommandLineTools/SDKs/MacOSX12.sdk/usr/include/c++/v1/locale:3569:35: error: too few arguments provided to function-like macro invocation
        return do_open(__nm, __loc);
                                  ^
/Library/Developer/CommandLineTools/SDKs/MacOSX12.sdk/System/Library/Perl/5.30/darwin-thread-multi-2level/CORE/perl.h:6924:9: note: macro 'do_open' defined here
#define do_open(g, n, l, a, rm, rp, sf) \
        ^

This was referenced Oct 3, 2022
@cho-m cho-m added the ready to merge PR can be merged once CI is green label Oct 4, 2022
@cho-m
Copy link
Member Author

cho-m commented Oct 4, 2022

I think this is ready to merge once CI finishes. macOS is all passing while Linux currently only failed on grokj2k, which should be fixed in #112269.

danielnachun
danielnachun previously approved these changes Oct 4, 2022
Copy link
Contributor

@danielnachun danielnachun left a comment

Choose a reason for hiding this comment

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

Everything is passing on macOS and as hoped the only Linux failure is grokj2k. Let get this merged.

@BrewTestBot
Copy link
Member

:shipit: @danielnachun has triggered a merge.

@BrewTestBot
Copy link
Member

⚠️ @danielnachun bottle publish failed.

@BrewTestBot BrewTestBot dismissed danielnachun’s stale review October 4, 2022 04:58

bottle publish failed

@BrewTestBot
Copy link
Member

:shipit: @danielnachun has triggered a merge.

@cho-m cho-m deleted the bump-perl-5.36.0-alt branch December 21, 2022 20:33
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-linux-self-hosted Build on Linux self-hosted runner CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. long build Set a long timeout for formula testing no long build conflict Do not allow merging other pull requests when files conflict with this one outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants