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: exclude version patch number in lib directory #84510

Closed
wants to merge 16 commits into from

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Sep 2, 2021

  • 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>?

Splitting off the change from #84191.

Directory structure using only major/minor version to avoid breakage on Perl patch releases. This is closer to how Arch Linux and macOS package it:


One thing that may be worth considering is setting up an installation structure for Perl formulae similar to other Linux distros and Homebrew's Python formulae so that we can easily pick up very common packages like XML::Parser rather than jumping through hoops with the PERL5LIB.

@cho-m cho-m added do not merge in progress Stale bot should stay away CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Sep 2, 2021
SMillerDev
SMillerDev previously approved these changes Sep 2, 2021
@cho-m cho-m added the CI-linux-self-hosted Build on Linux self-hosted runner label Sep 3, 2021
@carlocab carlocab requested a review from Bo98 September 3, 2021 03:49
@Bo98
Copy link
Member

Bo98 commented Sep 3, 2021

I've not looked into the grokj2k situation yet, but based on this PR alone this makes sense to me.

This is a backwards incompatible change, though would've eventually broke anyway when updating to 5.34.1 (if there even will be one - there's not one planned at the moment).

One thing that may be worth considering is setting up an installation structure for Perl formulae similar to other Linux distros and Homebrew's Python formulae so that we can easily pick up very common packages like XML::Parser rather than jumping through hoops with the PERL5LIB.

I'm open to some restructuring if it makes things easier but I'm not sure how that solves the issue described. XML::Parser does not ship with the Perl formula. We ship it on Linux with intltool and it requires PERL5LIB because it is installed into libexec.

@cho-m
Copy link
Member Author

cho-m commented Sep 3, 2021

The latter part is partly comparing to how Linux distros may package Perl modules and installed them into general vendor_perl. Also, how we have a few commonly used Python packages as formulae that install into general prefix rather than venv/libexec.

However, based on Homebrew's rules relating to CPAN modules, it probably doesn't align with formulae requirements. Overall, not too important of a topic and can be ignored.

I'm just noting that there is often some overhead when interacting with Perl formulae.


On separate note, after Linux CI finishes, I need to check on the libperl linkage for the revision-bumped formulae.

For example, current vim bottle on Linux only has following perl paths in RPATH

  • /home/linuxbrew/.linuxbrew/Cellar/perl/5.34.0/lib/perl5/5.34.0/x86_64-linux-thread-multi/CORE
  • /home/linuxbrew/.linuxbrew/opt/perl/lib

libperl.so is linked via the full versioned Cellar. Since I didn't change vim formula other than revision bump, I think this will still be an issue. If so, I need to see if the path can be manually set to opt path rather than the Cellar path being autodetected.

Also want to check weechat and mhonarc (and any other formulae with broken linkage that may be output from Linux CI).

EDIT:
No issue with mhonarc as it is just Perl scripts and doesn't link library. Version bump is due to default install directory being Perl's versioned site_perl.

weechat is similar to vim and uses Cellar path on Linux, but opt path on macOS:

  • libperl.so => /home/linuxbrew/.linuxbrew/Cellar/perl/5.34.0/lib/perl5/5.34.0/x86_64-linux-thread-multi/CORE/libperl.so
  • /usr/local/opt/perl/lib/perl5/5.34.0/darwin-thread-multi-2level/CORE/libperl.dylib

@Bo98
Copy link
Member

Bo98 commented Sep 3, 2021

In theory, that could potentially be fixed by doing one of:

  • Install libperl directly into lib.
    • Perl's build system does not support doing this.
  • Set -Dprefix to opt and -Dinstallprefix to Cellar.
    • -Dprivlib etc that we customise would also be set to the opt. Not sure if -Dinstallprivlib is necessary - it might just work automatically based on -Dprefix and -Dinstallprefix.
  • Hack Config.pm and/or Config_heavy.pl if all options fail.

@cho-m
Copy link
Member Author

cho-m commented Sep 3, 2021

I think Debian does a combination of first and last https://salsa.debian.org/perl-team/interpreter/perl/-/blob/debian-5.32/debian/rules#L168. Their libraries are provided as:

  • Package: libperl5.32
    • /usr/lib/x86_64-linux-gnu/libperl.so.5.32
    • /usr/lib/x86_64-linux-gnu/libperl.so.5.32.1
  • Package: libperl-dev
    • /usr/lib/x86_64-linux-gnu/libperl.a
    • /usr/lib/x86_64-linux-gnu/libperl.so

I'll probably try the 2nd option for now as it seems simpler and a smaller change from current bottles.

Though, it retains the demerit of some annoying libperl path, which requires a bit more work to get to once when we need it in a dependent formula (e.g. this is what I did in grokj2k to update RPATH).


Still need to go through some Linux failures after excluding the ones related to CI Java issue. And, need to wait for Self-Hosted queue to clear up a bit as any CI run will need to wait for Python3.9 and Boost PRs.

@cho-m
Copy link
Member Author

cho-m commented Sep 7, 2021

I locally tried adding installprefix for Linuxbrew and paths are now:

# perl -V:'install(privlib|archlib|vendorlib|vendorarch|sitelib|sitearch)'
installarchlib='/home/linuxbrew/.linuxbrew/Cellar/perl/5.34.0/lib/perl5/5.34/x86_64-linux-thread-multi';
installprivlib='/home/linuxbrew/.linuxbrew/Cellar/perl/5.34.0/lib/perl5/5.34';
installsitearch='/home/linuxbrew/.linuxbrew/Cellar/perl/5.34.0/lib/perl5/site_perl/5.34/x86_64-linux-thread-multi';
installsitelib='/home/linuxbrew/.linuxbrew/Cellar/perl/5.34.0/lib/perl5/site_perl/5.34';
installvendorarch='';
installvendorlib='';

# perl -V:'(privlib|archlib|vendorlib|vendorarch|sitelib|sitearch)'
archlib='/home/linuxbrew/.linuxbrew/opt/perl/lib/perl5/5.34/x86_64-linux-thread-multi';
privlib='/home/linuxbrew/.linuxbrew/opt/perl/lib/perl5/5.34';
sitearch='/home/linuxbrew/.linuxbrew/opt/perl/lib/perl5/site_perl/5.34/x86_64-linux-thread-multi';
sitelib='/home/linuxbrew/.linuxbrew/opt/perl/lib/perl5/site_perl/5.34';
vendorarch='';
vendorlib='';

In comparison, the macOS system Perl:

/usr/bin/perl -V:'install(privlib|archlib|vendorlib|vendorarch|sitelib|sitearch)'
installarchlib='/Library/Perl/Updates/5.30.2/darwin-thread-multi-2level';
installprivlib='/Library/Perl/Updates/5.30.2';
installsitearch='/Library/Perl/5.30/darwin-thread-multi-2level';
installsitelib='/Library/Perl/5.30';
installvendorarch='/Network/Library/Perl/5.30/darwin-thread-multi-2level';
installvendorlib='/Network/Library/Perl/5.30';/usr/bin/perl -V:'(privlib|archlib|vendorlib|vendorarch|sitelib|sitearch)'
archlib='/System/Library/Perl/5.30/darwin-thread-multi-2level';
privlib='/System/Library/Perl/5.30';
sitearch='/Library/Perl/5.30/darwin-thread-multi-2level';
sitelib='/Library/Perl/5.30';
vendorarch='/Network/Library/Perl/5.30/darwin-thread-multi-2level';
vendorlib='/Network/Library/Perl/5.30';

Also weechat rebuilt after Perl:

# ldd /home/linuxbrew/.linuxbrew/Cellar/weechat/3.2.1/lib/weechat/plugins/perl.so
	libperl.so => /home/linuxbrew/.linuxbrew/opt/perl/lib/perl5/5.34/x86_64-linux-thread-multi/CORE/libperl.so (0x0000004001a80000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x0000004001e11000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00000040021dc000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00000040023f9000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00000040025fd000)
	libcrypt.so.1 => /lib/x86_64-linux-gnu/libcrypt.so.1 (0x0000004002907000)
	/lib64/ld-linux-x86-64.so.2 (0x0000004000000000)

Comment on lines +134 to +136
inreplace Dir[libexec/"bin/{perl-build,config_data}"] do |s|
s.sub! %r{^#!#{HOMEBREW_PREFIX}/Cellar/perl/[^/]+/bin/perl}o, "#!#{Formula["perl"].opt_bin}/perl"
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.

EDIT: Re-added comment after force-push.

I think the perl-build build script replaces the Perl shebang with realpath of Homebrew Perl. I couldn't find another way to deal with this:

  • detected_perl_shebang will only return system /usr/ Perl paths
  • Trying to inreplace Formula["perl"].bin fails as it returns opt_bin:
    Error: An exception occurred within a child process:
      Utils::Inreplace::Error: inreplace failed
    /home/linuxbrew/.linuxbrew/Cellar/perl-build/1.32/libexec/bin/perl-build:
      expected replacement of #<Pathname:/home/linuxbrew/.linuxbrew/opt/perl/bin> with #<Pathname:/home/linuxbrew/.linuxbrew/opt/perl/bin>
    /home/linuxbrew/.linuxbrew/Cellar/perl-build/1.32/libexec/bin/config_data:
      expected replacement of #<Pathname:/home/linuxbrew/.linuxbrew/opt/perl/bin> with #<Pathname:/home/linuxbrew/.linuxbrew/opt/perl/bin>
    

@cho-m cho-m force-pushed the perl-ignore-patch branch 3 times, most recently from 06c31ea to 47fa38e Compare September 7, 2021 20:35
@cho-m
Copy link
Member Author

cho-m commented Sep 7, 2021

freeradius-server

Due to collectd being installed on CI runner before freeradius-server, it is being detected by configure. I can't find a way to prevent it and have tried all of: --without-collectd, --without-collectdclient, --with-collectd=no, --with-collectdclient=no, --with-collectdclient-lib-dir=, --with-collectdclient-lib-dir=no, --with-collectdclient-lib-dir=/dev/null

Since most repos don't enable this feature and Debian specifically has comment on there being issues (https://salsa.debian.org/debian/freeradius/-/blob/master/debian/control#L6), I want to avoid this being linked in new bottle. At this point, I may just need to manually inreplace the configure or configure.ac.

mhonarc

With Perl directory changes, mhonarc is now failing to pick the correct directories. It is now trying to set directories relative to prefix (e.g. prefix/man rather than prefix/share/man and prefix/lib rather than prefix/lib/perl5/site_perl/VERSION. Not sure exact reason. The related logic may be:

    my $cfg_prefix        = interpolate_path($Config{'prefix'});
    $DefValues{'binpath'} = interpolate_path($Config{'installbin'});
    $DefValues{'libpath'} = interpolate_path($Config{'installsitelib'});
    $DefValues{'manpath'} = interpolate_path($Config{'installman1dir'});
    if (defined($OptValues{'prefix'})) {
	$DefValues{'binpath'} = join($DIRSEP, $OptValues{'prefix'}, 'bin')
	    unless $DefValues{'binpath'} =~
		   s/^\Q$cfg_prefix/$OptValues{'prefix'}/o;
	$DefValues{'libpath'} = join($DIRSEP, $OptValues{'prefix'}, 'lib')
	    unless $DefValues{'libpath'} =~
		   s/^\Q$cfg_prefix/$OptValues{'prefix'}/o;
	$DefValues{'manpath'} = join($DIRSEP, $OptValues{'prefix'}, 'man')
	    unless $DefValues{'manpath'} =~
		   s/^\Q$cfg_prefix/$OptValues{'prefix'}/o;
	$DefValues{'docpath'} = join($DIRSEP, $OptValues{'prefix'}, 'doc');

@cho-m cho-m removed do not merge in progress Stale bot should stay away labels Sep 9, 2021
@cho-m
Copy link
Member Author

cho-m commented Sep 9, 2021

Linux failures (Error: 52 failed steps!) are mostly CI issues and previously seen issues. May need to make sure about rbenv (EDIT: from logs, also seems unrelated), but everything else seems unrelated.

brew test --retry --verbose rbenv
21 due to reoccurring Java CI issue
brew test --retry --verbose aspectj
brew test --retry --verbose bbtools
brew test --retry --verbose beagle
brew test --retry --verbose bnfc
brew test --retry --verbose cfr-decompiler
brew test --retry --verbose dita-ot
brew test --retry --verbose duck
brew test --retry --verbose gradle
brew test --retry --verbose groovy
brew test --retry --verbose groovysdk
brew test --retry --verbose hadoop
brew test --retry --verbose htmlcleaner
brew test --retry --verbose javacc
brew test --retry --verbose jdnssec-tools
brew test --retry --verbose jmeter
brew test --retry --verbose joshua
brew test --retry --verbose jruby
brew test --retry --verbose kaitai-struct-compiler
brew test --retry --verbose leiningen
brew test --retry --verbose tika
brew test --retry --verbose umple
24 due to no space left on CI runner
brew install --only-dependencies grpc-swift
brew install grpc-swift
brew install infer
brew install --only-dependencies lima
brew install lima
brew install llvm@7
brew install llvm@8
brew install llvm@9
brew install mercury
brew install mingw-w64
brew test --retry --verbose nexus
brew install nushell
brew test --retry --verbose pyoxidizer
brew install qemu
brew install root
brew install swift
brew install --only-dependencies swift-protobuf
brew install swift-protobuf
brew install --only-dependencies swift-sh
brew install swift-sh
brew install tesseract-lang
brew install --only-dependencies --include-test tree-sitter
brew test --retry --verbose tree-sitter
brew install widelands
6 failures also seen in Python 3.9.7 PR #84283
brew test --retry --verbose buku
brew test --retry --verbose julia
brew test --retry --verbose kafka
brew test --retry --verbose mrbayes
brew test --retry --verbose parquet-cli
brew test --retry --verbose streamlink

@cho-m cho-m added the ready to merge PR can be merged once CI is green label Sep 10, 2021
-Dman1dir=#{man1}
-Dman3dir=#{man3}
-Dman1dir=#{opt_share}/man/man1
-Dman3dir=#{opt_share}/man/man3
-Duseshrplib
-Duselargefiles
-Dusethreads
]
args << "-Dsed=/usr/bin/sed" if OS.mac?
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this, as it's a no-op. Could probably wait for a follow-up PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add this with I get a chance to rebase. vim was updated so the PR needs to be re-run for new bottles.

@mislav mislav mentioned this pull request Sep 28, 2021
@cho-m cho-m added the in progress Stale bot should stay away label Oct 10, 2021
@SMillerDev
Copy link
Member

Can you rebase this so we can merge it @cho-m ?

@cho-m cho-m removed the ready to merge PR can be merged once CI is green label Oct 27, 2021
@cho-m
Copy link
Member Author

cho-m commented Oct 27, 2021

Can you rebase this so we can merge it @cho-m ?

I'll try to rebase the PR this week when I get some time.

Hopefully can avoid any merge conflicts with Monterey bottling.

@iMichka iMichka removed the in progress Stale bot should stay away label Nov 23, 2021
@carlocab carlocab removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Dec 8, 2021
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Dec 30, 2021
@github-actions github-actions bot closed this Jan 8, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Feb 8, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2022
@cho-m cho-m deleted the perl-ignore-patch branch April 24, 2022 21:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. outdated PR was locked due to age stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants