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.

Sign up for GitHub

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
Closed

asciiquarium: fix Big Sur building #66485

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce71e0f
asciiquarium: fix Big Sur building
mitchblank Dec 8, 2020
5f3a69b
disable test under CI; only passes manually
mitchblank Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions Formula/asciiquarium.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ class Asciiquarium < Formula
homepage "https://robobunny.com/projects/asciiquarium/html/"
url "https://robobunny.com/projects/asciiquarium/asciiquarium_1.1.tar.gz"
sha256 "1b08c6613525e75e87546f4e8984ab3b33f1e922080268c749f1777d56c9d361"
license "GPL-2.0"
revision 1
license "GPL-2.0-or-later"
revision 2

livecheck do
url "https://robobunny.com/projects/asciiquarium/"
Expand All @@ -21,9 +21,11 @@ class Asciiquarium < Formula
sha256 "6b20abf264f40c7123e40f0f34cfc11f0c12a03b1a74a324e3f3a7ae75e94f3f" => :yosemite
end

uses_from_macos "ncurses"

resource "Curses" do
url "https://cpan.metacpan.org/authors/id/G/GI/GIRAFFED/Curses-1.34.tar.gz"
sha256 "808e44d5946be265af5ff0b90f3d0802108e7d1b39b0fe68a4a446fe284d322b"
url "https://cpan.metacpan.org/authors/id/G/GI/GIRAFFED/Curses-1.37.tar.gz"
sha256 "74707ae3ad19b35bbefda2b1d6bd31f57b40cdac8ab872171c8714c88954db20"
end

resource "Term::Animation" do
Expand All @@ -37,6 +39,39 @@ def install
resources.each do |r|
r.stage do
system "perl", "Makefile.PL", "INSTALL_BASE=#{libexec}"
if (r.name == "Curses") && (MacOS.version >= :big_sur)
# 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
Comment on lines +43 to +68
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.

mk_lines = File.readlines("Makefile")
if mk_lines.grep(/^LDLOADLIBS\s*=/).empty?
mv "Makefile", "Makefile.orig"
File.open("Makefile", "w") { |f| f << "LDLOADLIBS = -lcurses\n" << mk_lines.join << "\n" }
end
end
system "make"
system "make", "install"
end
Expand All @@ -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.


# This is difficult to test because:
# - There are no command line switches that make the process exit
# - The output is a constant stream of terminal control codes
Expand Down