Conversation
|
It seems like it has trouble finding native-image tool as the java home path is openjdk instead of graalvm-jdk. I will investigate how to properly build it |
change the javahome like this ENV["JAVA_HOME"] = if OS.mac?
Formula["graalvm"].opt_libexec/"graalvm.jdk/Contents/Home"
else
Formula["graalvm"].opt_libexec
endand see #275893 for other stuff |
7c55064 to
ef51504
Compare
687c061 to
946f86b
Compare
|
@GunniBusch Thank you for the feedback. It seems now that all of the checks are passing. I was just thinking about the previous discussions of supporting the mac intel with graalvm. As that is not possible as decided previously. The following snippet might work out as a way to provide end-users a working cli app for mac with intel by building for that one with openjdk and fat-jar instead of graalvm. All Linux with intel and arm, mac with arm can still be built with graalvm and then all of the cases would have been covered. Do you think that would be ok? What do you think of the snippet below: if OS.mac? && Hardware::CPU.intel?
depends_on "maven" => :build
depends_on "openjdk"
def install
system "mvn", "clean", "package", "-Pfat-jar", "-DskipTests=true"
libexec.install "target/crip.jar"
bin.write_jar_script libexec/"crip.jar", "crip"
end
else
depends_on "graalvm" => :build
depends_on "maven" => :build
on_linux do
depends_on "zlib-ng-compat"
end
def install
ENV["JAVA_HOME"] = if OS.mac?
Formula["graalvm"].opt_libexec/"graalvm.jdk/Contents/Home"
else
Formula["graalvm"].opt_libexec
end
native_image_env = ENV.keys.grep(/^HOMEBREW_/).map { |key| "-E#{key}" }
ENV.prepend "NATIVE_IMAGE_OPTIONS", native_image_env.join(" ")
system "mvn", "clean", "package", "-Pnative-image", "-DskipTests=true"
bin.install "target/crip"
end
end |
946f86b to
a686be0
Compare
f5a6a5f to
dc20824
Compare
Technically you could do that, but end of day this is something a maintainer would need to decide. When I did cljfmt I opted out, due to deprecation of intel support hombrew wise. But regarding HOW to do this, your best bet would be https://docs.brew.sh/Formula-Cookbook#specifying-other-formulae-as-dependencies and https://docs.brew.sh/Formula-Cookbook#handling-different-system-configurations, |
|
Ah, I see. Homebrew will drop support for mac with intel in September 2026. Then it does not make sense to have this kind of logic actually. Let me revert my changes. Thank you for pointing out |
dc20824 to
f8bdc58
Compare
|
looks good to me, but I am not a maintainer so I cant review nor make final decisions |
| native_image_env = ENV.keys.grep(/^HOMEBREW_/).map { |key| "-E#{key}" } | ||
| ENV.prepend "NATIVE_IMAGE_OPTIONS", native_image_env.join(" ") |
There was a problem hiding this comment.
What is this for? It seems like it would bake credentials into the build as well as other options.
There was a problem hiding this comment.
I have no idea to be honest. @GunniBusch suggested it,.maybe he can give some context.
The build was failing and I think it worked after added those options, but I am not sure anymore as I tried a lot to get the build working. I can retry without the options and check whether it would make a difference
There was a problem hiding this comment.
What is this for? It seems like it would bake credentials into the build as well as other options.
Good question,.. No honestly, this is because native-image removes all envs, so we need to allow homebrew envs to have the superenv stuff working.
And no, I tried removing the superenv, but this caused issues because then native image couldn't find zlib-ng library (or bascially the linker couldn't). And I tried like one hour to debug this and finally decided to just allow all Homebrew envs.
But this doesn't just bake envs into the build (unless the author designed that to do, but that is not unique to graalvm or this setup) and yes there is an issue about that in graalvm. oracle/graal#8639
This is similar to for example bazel which also requires users to set like --*-action-env=ENV if they want to expose envs.
@SMillerDev I don't want to say this in public, as to not set ideas, so I keep vague, but the core of your concern, if applied to every ci run, would pose other unintended issues.
There was a problem hiding this comment.
I removed it and the build is failing, see here for the details: https://github.com/Homebrew/homebrew-core/actions/runs/23988009425/job/69963224388?pr=275950
I reverted back to keep the below snippet:
native_image_env = ENV.keys.grep(/^HOMEBREW_/).map { |key| "-E#{key}" }
ENV.prepend "NATIVE_IMAGE_OPTIONS", native_image_env.join(" ")
So the build should be passing again
There was a problem hiding this comment.
Can't we just set specific keys? Because this would pass things like HOMEBREW_GITHUB_API_TOKEN which I don't think we should do.
There was a problem hiding this comment.
I am testing those envs and we don't need them all, just a subset. I will keep you guys posted of the exact list of envs
It seems like we only need the following list:
HOMEBREW_RUBY_PATH
HOMEBREW_CC
HOMEBREW_CELLAR
HOMEBREW_OPT
HOMEBREW_LIBRARY_PATHS
HOMEBREW_RPATH_PATHSI am not so familar with how to properly code in Ruby, but I don't think it would be ideal to have the snippet below for every Formula which uses GraalVM, right? Not quite sure whether it can be configured at a higher level. What do you guys think?
brew_envs = %w[
HOMEBREW_RUBY_PATH
HOMEBREW_CC
HOMEBREW_CELLAR
HOMEBREW_OPT
HOMEBREW_LIBRARY_PATHS
HOMEBREW_RPATH_PATHS
]
native_image_env = brew_envs.map { |key| "-E#{key}" }
ENV.prepend "NATIVE_IMAGE_OPTIONS", native_image_env.join(" ")There was a problem hiding this comment.
I'm still a bit confused what we're doing with these. Does graal use variables with a homebrew prefix? Or am I misunderstanding what -EHOMEBREW_RUBY_PATH would do?
There was a problem hiding this comment.
I'm still a bit confused what we're doing with these. Does graal use variables with a homebrew prefix? Or am I misunderstanding what
-EHOMEBREW_RUBY_PATHwould do?
Lets just say, its graalvm god know what it does, but no, graalvm clears envs that are not explicitly allowed, which you do with -E. And because I was not able to get native-image to work setting LD-FLAGS manually, the only option is to use shims. But shims need HOMEBREW_ envs so we need to whitelist them.
And yes this is a feature. At least bug reports were dismissed saying this is a feature not a bug.
There was a problem hiding this comment.
But maybe lets move this out of this pr, as this is maybe relevant to other prs, as well, and also I believe there are more maintainers who have a opinion here but talks happen in like 3 prs, and I dont think we want to end up with 3 different solutions to the same problem, especially since using shims has its perks on its own, and I even think this might also apply to other build systems that strip envs and have options to whitelist them
There was a problem hiding this comment.
Hi guys any updated on this topic and also what the proper solution would be?
ee1ab53 to
81e0c74
Compare
3abc438 to
7638b1d
Compare
bc226b9 to
25b164f
Compare
d38e377 to
86f9d66
Compare
86f9d66 to
c663ac9
Compare
HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>?brew test <formula>?brew audit --strict <formula>(after doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it passbrew audit --new <formula>?