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

asciiquarium: fix Big Sur building #66485

Closed
wants to merge 2 commits into from

Conversation

mitchblank
Copy link
Contributor

The problem wasn't with asciiquarium itself, but with building the Curses perl resource. In turn, this is due to an issue
with the Apple-provided perl MakeMaker as described here: Perl-Toolchain-Gang/ExtUtils-MakeMaker#381

Because of the complicated symbol detection inside the Curses resource working around this issue is a bit messy.

While working on it I updated the Curses resource to the latest upstream version and thus had to bump the formula revision.

The problem wasn't with asciiquarium itself, but with building
the Curses perl resource.  In turn, this is due to an issue
with the Apple-provided perl MakeMaker as described here:
  Perl-Toolchain-Gang/ExtUtils-MakeMaker#381

Because of the complicated symbol detection inside the Curses
resource working around this issue is a bit messy.

While working on it I updated the Curses resource to the latest
upstream version and thus had to bump the formula revision.
Comment on lines +43 to +68
# Work around a 11.0 problem in MakeMaker:
# https://github.com/Perl-Toolchain-Gang/ExtUtils-MakeMaker/issues/381
#
# This issue is that due to the changes to how libraries
# exist (or, rather, DON'T exist) on the filesystem, usual ways
# of detecting libraries do not work. This extends even
# to the MakeMaker that comes bundled with Apple's own
# copy of perl (at least as of OS 11.0.1)
#
# The result is that Makefile.PL generates a Makefile that
# does not contain the expected "LDLOADLIBS = -lcurses" line.
# The tell is when it prints the diagnostic:
# Warning (mostly harmless): No library found for -lcurses
# Since it doesn't find the library, it assumes no flag is needed.
#
# Normally we could probably work around this by just specifying
# an explicit value of LDLOADLIBS on the "make" command line,
# but that doesn't work here. That is because the Makefile
# runs the perl script "test.syms" which then opens and re-parses
# "Makefile" to find these values and then uses them to introspect
# the system's curses library. If it doesn't find the right
# value of LDLOADLIBS *inside the Makefile* it won't find any
# symbols and the build will end up not working.
#
# Therefore we need to actually munge "Makefile" after "Makefile.PL"
# runs but before "make" does
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Work around a 11.0 problem in MakeMaker:
# https://github.com/Perl-Toolchain-Gang/ExtUtils-MakeMaker/issues/381
#
# This issue is that due to the changes to how libraries
# exist (or, rather, DON'T exist) on the filesystem, usual ways
# of detecting libraries do not work. This extends even
# to the MakeMaker that comes bundled with Apple's own
# copy of perl (at least as of OS 11.0.1)
#
# The result is that Makefile.PL generates a Makefile that
# does not contain the expected "LDLOADLIBS = -lcurses" line.
# The tell is when it prints the diagnostic:
# Warning (mostly harmless): No library found for -lcurses
# Since it doesn't find the library, it assumes no flag is needed.
#
# Normally we could probably work around this by just specifying
# an explicit value of LDLOADLIBS on the "make" command line,
# but that doesn't work here. That is because the Makefile
# runs the perl script "test.syms" which then opens and re-parses
# "Makefile" to find these values and then uses them to introspect
# the system's curses library. If it doesn't find the right
# value of LDLOADLIBS *inside the Makefile* it won't find any
# symbols and the build will end up not working.
#
# Therefore we need to actually munge "Makefile" after "Makefile.PL"
# runs but before "make" does
# Work around a 11.0 problem in MakeMaker:
# https://github.com/Perl-Toolchain-Gang/ExtUtils-MakeMaker/issues/381

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 know the comment block is long, but it provides a lot of context that took me a long time to figure out. Just referencing that MakeMaker bug does nothing to explain the intricacies of why we have to munge Makefile for test.syms's benefit which is very non-obvious.

Copy link
Member

Choose a reason for hiding this comment

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

@mitchblank, perhaps you'd like to document this in a gist that you can link to in this comment block? Maybe that will achieve a compromise between keeping the formula clean but also having the necessary information easy to access.

@mitchblank
Copy link
Contributor Author

Damn, the 10.15 and 11.0 CI tests failed, even though 11.0 passes manually. This seems eerily similar to the issue I had when I tried to add a unit test to #66397 and it failed because it wasn't running with stdin attached to a tty.

Here the test that @dnicolson originally wrote is explicitly trying to allocate a pty, but that is failing on 10.15 and 11.0 but passing on 10.14. I suspect that this is another CI sandbox issue, since it definitely works when you run brew test directly. Is there a way to mark a test as non-CI eligible, since I don't think we can test this meaningfully if we can't allocate a pty somehow.

@mitchblank
Copy link
Contributor Author

Is there a way to mark a test as non-CI eligible

Never mind, I found the ENV["CI"] trick that other formula use for this

@@ -53,6 +88,8 @@ def install
end

test do
return if ENV["CI"] # CI environment post-10.14 can't use ptys
Copy link
Member

Choose a reason for hiding this comment

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

This is a strange situation because the test ran and worked when we added the catalina bottle, and all sandboxing should also happen locally. The issues other formulae are having are usually resource based, not sandbox.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't fully understand this either... looking at other formula there are PTY.spawn-using formula that have :big_sur bottles. Unless those were overridden they must have worked.

Yet the ones I've been trying today have definitely been failing on 10.15 and 11 every time (but run locally on 11 no problem) Maybe something regressed in the CI infra itself and pty-using tests that used to pass on 10.15/11 are now failing?

Copy link
Member

Choose a reason for hiding this comment

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

We definitely don't want to disable tests, or remove them. The CI should be able to perform terminal-based tests, if we have specific failures we should investigate or open a brew issue.

@mitchblank
Copy link
Contributor Author

BTW a similar MakeMaker issue is likely the cause of Term::ReadKey not building which is the source of failures on rex, po4a, and creduce

I'll probably dig into those later this week unless someone beats me to them.

@BrewTestBot
Copy link
Member

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.

@BrewTestBot BrewTestBot added stale No recent activity and removed stale No recent activity labels Dec 30, 2020
mitchblank added a commit to mitchblank/homebrew-core that referenced this pull request Jan 3, 2021
When called *without* the "-l" option, extract_url is supposed to
be a curses-based app that lets you interactively pick which URL
to open.  However if there is an issue with the perl Curses library
it just silently falls back to "-l" mode.

This is what was happening on Big Sur; since Curses was failing
in the same weird way as in the asciiquarium Forumla (see Homebrew#66485)
the package appeared to build OK but the curses-based UI didn't actually
work.

I'm importing the same nasty workaround I made to get asciiquarium
happy.  I also added some tests that would catch if this behavior
happened again -- both by testing that our perl resources actually
load OK and by running the TUI in a pty.  The pty test isn't CI-friendly
(at least on the post-10.14 CI environments) but the resource-loading
test should give CI some decent coverage for this condition.

Also bump up the versions of the resources while I'm here, and therefore
bumping the revision.
@BrewTestBot
Copy link
Member

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.

@BrewTestBot BrewTestBot added the stale No recent activity label Jan 21, 2021
@mitchblank mitchblank mentioned this pull request Feb 17, 2021
5 tasks
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 28, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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