-
-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Conversation
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.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
Never mind, I found the |
@@ -53,6 +88,8 @@ def install | |||
end | |||
|
|||
test do | |||
return if ENV["CI"] # CI environment post-10.14 can't use ptys |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
BTW a similar MakeMaker issue is likely the cause of I'll probably dig into those later this week unless someone beats me to them. |
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. |
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.
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. |
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.