nu: fix build on Linux #104928
nu: fix build on Linux #104928
Conversation
BrewTestBot
added
no ARM bottle
Formula/nu.rb
Outdated
| def install | ||
| ENV.delete("SDKROOT") if MacOS.version < :sierra | ||
| ENV["PREFIX"] = prefix | ||
| # Some gnustep-make scripts located in libexecA. | ||
| ENV.append_path "PATH", Formula["gnustep-make"].libexec unless OS.mac? |
There was a problem hiding this comment.
Can we move this into the unless block below?
Formula/nu.rb
Outdated
| "-L#{Formula["swift"].libexec}/lib/swift/linux" | ||
| "-Wl,-rpath,#{Formula["swift"].libexec}/lib/swift/linux" | ||
| ] | ||
| inreplace "Makefile", "LDFLAGS = ", "LDFLAGS = #{ldflags.join(" ")}" |
There was a problem hiding this comment.
We can't pass these LDFLAGS to make? Or use #change_make_var!?
There was a problem hiding this comment.
I missed this comment earlier. Unfortunately the line I was replacing is the one which seems to reset the LDFLAGS so passing it in doesn't work. Further complicating matters, the way the LDFLAGS are constructed in the Makefile seems incompatible with change_make_var:
LDFLAGS =
LDFLAGS += $(FRAMEWORKS)
LDFLAGS += $(LIBS)
LDFLAGS += $(LIBDIRS)
LDFLAGS += $(FFI_LIB)
ifeq ($(SYSTEM), Darwin)
else
#LDFLAGS += $(shell gnustep-config --base-libs)
ifneq ($(SYSTEM), SunOS)
LDFLAGS += -Wl,--rpath -Wl,/usr/local/lib
#LDFLAGS += `gnustep-config --debug-flags`
LDFLAGS += -L/usr/local/lib
LDFLAGS += -ldispatch
LDFLAGS += -lgnustep-base
# LDFLAGS += -lgnustep-gui
LDFLAGS += -L/usr/lib/GNUstep
So as far as I can tell inreplace seems to be the only way for this to work.
There was a problem hiding this comment.
And as soon as I pasted that, I realized I had missed that LIBDIRS is not overridden in the Makefile. I was able to pass in the flags just by setting it in env, which is much cleaner than the inreplace. I'll push this change now.
There was a problem hiding this comment.
If you want to use LDFLAGS just add the flags you want to ENV.ldflags then pass "LDFLAGS=#{ENV.ldflags}" to make.
There was a problem hiding this comment.
I tried that and it doesn't seem to work because the first line in that part of the Makefile is LDFLAGS = , which I believe has the effect of resetting the LDFLAGS even if I pass them in as an argument. I think if they were just using += for all the assignments it would work as you described and that's indeed what is done with LIBDIRS, hence why it works.
Formula/nu.rb
Outdated
| ] | ||
| inreplace "Makefile", "LDFLAGS = ", "LDFLAGS = #{ldflags.join(" ")}" | ||
| # Don't hard code path to clang. | ||
| inreplace "tools/nuke", "/usr/bin/clang", ENV.cc |
There was a problem hiding this comment.
We might want to do this on macOS too.
Formula/nu.rb
Outdated
| inreplace "objc/NuBridge.h", "x86_64-linux-gnu/", "" | ||
| inreplace "objc/NuBridge.m", "x86_64-linux-gnu/", "" | ||
| inreplace "objc/Nu.m", "x86_64-linux-gnu/", "" |
There was a problem hiding this comment.
Pass an Array to inreplace since these are all the same.
Formula/nu.rb
Outdated
|
|
||
| # Nu does not (yet) support the GNUstep 2.0 Objective C runtime so it must use 1.9. | ||
| # If this flag is not changed from its default, the linker fails to find some symbols. | ||
| inreplace "Nukefile", "-fgnu-runtime", "-fobjc-runtime=gnustep-1.9" |
There was a problem hiding this comment.
Hard-coding the 1.9 here seems really prone to bitrot.
There was a problem hiding this comment.
As documented here: https://github.com/gnustep/libobjc2, this is a defined standard whose version won't change over time. We should check with new releases if the 2.0 run time is supported. Maybe I should add this to comment?
There was a problem hiding this comment.
Yes. Along with links to documentation, and how to tell when nu supports the newer runtime.
There was a problem hiding this comment.
I have an interesting update to this. The problem was the the Nukefile was also setting -DGNU_RUNTIME=1, which will result in the compiler trying to find symbols for the GNU runtime. However if I remove -DGNU_RUNTIME=1 and -fgnu-runtime from the CFLAGS, it automatically picks the the 1.9 run time without having to explicitly specify it.
I believe this is essentially aligning it with what happens on macOS and should avoid bitrot as we're just removing some irrelevant flags. If they decide to move to the Objective-C 2.0 runtime, upstream will have to remove these flags on Linux and the workaround will not be needed anymore. I will update the comment to explain this.
danielnachun
force-pushed
the
nu
branch
2 times, most recently
from
8a3ed2c to
ca1424f
Compare
|
🤖 A scheduled task has triggered a merge. |
|
ARM build failure is an upstream issue -- they're still looking for a Filed programming-nu/nu#100. |
brew install --build-from-source <formula>, where<formula>is the name of the formula you're submitting?brew test <formula>, where<formula>is the name of the formula you're submitting?brew audit --strict <formula>(after doingbrew install --build-from-source <formula>)? If this is a new formula, does it passbrew audit --new <formula>?