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: depend on libxcrypt #108663

Closed
wants to merge 19 commits into from
Closed

Conversation

danielnachun
Copy link
Contributor

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

@danielnachun danielnachun added CI-skip-recursive-dependents Pass --skip-recursive-dependents to brew test-bot. CI-skip-dependents Pass --skip-dependents to brew test-bot. labels Aug 23, 2022
@danielnachun
Copy link
Contributor Author

It also looks like perl needs libnsl on Linux, because that is no longer of glibc. I'll update this to include that.

Formula/perl.rb Outdated
Comment on lines 31 to 36
on_linux do
depends_on "libnsl"
end
Copy link
Member

Choose a reason for hiding this comment

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

There is no linkage with libnsl, so this dependency doesn't seem to do anything. The previous run without this also did not register linkage with any libnsl, as far as I can tell.

If linkage with a system libnsl is not desired, then brew linkage needs to be adjusted to make sure it errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh weird, I guess it depends on the system then. On the OS I was testing this on it tried to find libnsl but I guess it doesn't do so depending on the host.

I think we actually need to adjust brew linkage to reject both libnsl.so.1 and libcrypt.so.1 as those are both still in the allowlist right now.

I guess better question now is, how do we stop perl from trying to link to libnsl if we don't want to add it as a dependency? It may be the case that this only happens when building from source on older systems but I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am okay with dropping libnsl for now so we can merge the libxcrypt dependency as that is more critical. Does that sound okay?

Copy link
Member

Choose a reason for hiding this comment

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

I guess better question now is, how do we stop perl from trying to link to libnsl if we don't want to add it as a dependency?

It might be a configure argument. If there isn't, I'd leave it alone. It's not really that important to stop opportunistic linkage, especially if it doesn't result in a build failure of some kind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it does result in a build failure which was why I wanted to fix this. Sorry I should have said that earlier!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regrettably the Configure script hardcodes the list of libraries it searches for and there doesn't seem to be a way to change this, so I just removed it from the list with inreplace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nvm, I dug up some old stuff from Linuxbrew on how to override libswanted. The syntax is a little wacky but it does the job and avoids any use of inreplace.

@danielnachun danielnachun force-pushed the perl branch 3 times, most recently from c70007f to 4de3db9 Compare August 23, 2022 19:19
Formula/perl.rb Outdated Show resolved Hide resolved
Formula/perl.rb Show resolved Hide resolved
@danielnachun danielnachun force-pushed the perl branch 2 times, most recently from 32e01d1 to 24d93ae Compare August 24, 2022 17:50
@carlocab carlocab removed CI-skip-recursive-dependents Pass --skip-recursive-dependents to brew test-bot. CI-skip-dependents Pass --skip-dependents to brew test-bot. labels Aug 25, 2022
@danielnachun danielnachun added CI-linux-self-hosted Build on Linux self-hosted runner long build Set a long timeout for formula testing CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. labels Aug 25, 2022
@carlocab carlocab added CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. and removed CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-linux-self-hosted Build on Linux self-hosted runner labels Aug 25, 2022
@danielnachun danielnachun added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Aug 26, 2022
@carlocab carlocab removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Aug 26, 2022
@danielnachun danielnachun removed the CI-linux-self-hosted Build on Linux self-hosted runner label Aug 26, 2022
@danielnachun
Copy link
Contributor Author

This is the list of failures:

Cellar reference to perl interpreter:

  • get-flash-videos
  • perl-build

Cellar reference to libperl.so:

  • collectd
  • freeradius-server
  • gnumeric
  • grokj2k
  • irssi
  • pidgin
  • postgresql
  • postgresql@10
  • postgresql@11
  • postgresql@12
  • postgresql@13
  • rxvt-unicode
  • uwsgi
  • v
  • vim
  • weechat

Unrelated

I will revision bump the formula with cellar references but I'm also going to open a tracking issue for these cases because none of them should require a revision bump when we revision bump Perl. We already do a really good job replacing cellar references but a handful of formulae have managed to evade our automated checks. If it turns out that we can fix some or all of these issues in brew instead of homebrew-core, we should prefer that when possible. But at the moment it's hard to say if these cellar references are getting through because of consistent issues or for reasons that are idiosyncratic to each formula.

@danielnachun danielnachun force-pushed the perl branch 2 times, most recently from 8a08ae7 to ce62508 Compare August 27, 2022 08:35
@danielnachun danielnachun added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. and removed CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. long build Set a long timeout for formula testing labels Aug 27, 2022
@carlocab carlocab added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Sep 2, 2022
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Something I don't understand here:

Why do all these formulae need revision bumps for perl to depend on libxcrypt?

Does the ABI of libperl.so change as a consequence of this?

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Ah, I see. These link with perl through a Cellar path. But that seems to have been fixed by your perl changes.

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@@ -3,6 +3,7 @@ class V < Formula
homepage "https://github.com/rupa/v"
url "https://github.com/rupa/v/archive/v1.1.tar.gz"
sha256 "6483ef1248dcbc6f360b0cdeb9f9c11879815bd18b0c4f053a18ddd56a69b81f"
revision 1
Copy link
Member

Choose a reason for hiding this comment

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

This formula is just a script and a manpage, so the rev bump seems like it was a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I think confusion was that it failed during test because vim was broken. Had I run a test where vim was revision bumped but v was not, it would have succeeded. We'll be able to skip revision bumping this when we upgrade to Perl 5.36.

Copy link
Member

Choose a reason for hiding this comment

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

Well, that's why we rev bump linkage failures before touching any test failures.

@danielnachun
Copy link
Contributor Author

Ah, I see. These link with perl through a Cellar path. But that seems to have been fixed by your perl changes.

That's what I'm hoping we achieved here, although the only way to find for sure is to do a test revision bump and make sure nothing breaks. I might try that quickly since it doesn't need a long timeout label, and would tell us if we missed anything here.

@danielnachun danielnachun removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Sep 2, 2022
@danielnachun danielnachun deleted the perl branch September 2, 2022 17:28
@cho-m cho-m mentioned this pull request Oct 2, 2022
6 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 3, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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