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

babashka: 1.3.186 -> 1.3.188, buildGraalNativeImage: remove NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION #282901

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

Sohalt
Copy link
Contributor

@Sohalt Sohalt commented Jan 22, 2024

Description of changes

Since babashka/babashka@6052b9b babashka uses -E arguments to build-native-image, which are incompatible with our use of NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION.
Derivations using buildGraalvmNativeImage should specify environment variables needed during compliation using -E in nativeImageBuildArgs.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

cc maintainers: @thiagokokada @jlesquembre @bennyandresen @bhougland @DerGuteMoritz @ericdallo @babariviere @stelcodes @rtimush @sg-qwt

--

Add a 👍 reaction to pull requests you find important.

…R_SANITATION

Packages that require access to environment variables in the build
should specify these using `-E` arguments in `nativeImageBuildArgs` or
using a `native-image.properties` as described here:
https://www.graalvm.org/22.1/reference-manual/native-image/BuildConfiguration/#embedding-a-configuration-file

Specifying NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION=true breaks with
packages that list their environment variables explicitly in `native-image.properties`.
Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

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

Is this PR missing something? Because I am not seeing the removal of NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION.

But anyway, this will not work because we depend in NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION for the NIX_* that we will pass to the build environment and GraalVM will ignore without NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION. It may work in Linux in some cases but definitely will not work in Darwin because we need to pass macOS libraries via NIX_LD_FLAGS.

@Sohalt
Copy link
Contributor Author

Sohalt commented Jan 22, 2024

Is this PR missing something? Because I am not seeing the removal of NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION.

Sorry, I forgot to push…

But anyway, this will not work because we depend in NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION for the NIX_* that we will pass to the build environment and GraalVM will ignore without NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION. It may work in Linux in some cases but definitely will not work in Darwin because we need to pass macOS libraries via NIX_LD_FLAGS.

Is this the case for builds with native-image? Which NIX_* variables do we need specifically? Can you try to build this PR on MacOS? If it's only NIX_LD_FLAGS we can pass that along using -ENIX_LD_FLAGS in nativeImageBuildArgs.

@thiagokokada
Copy link
Contributor

thiagokokada commented Jan 22, 2024

Is this the case for builds with native-image?

Yes, this is the case for builds with native-image

Can you try to build this PR on MacOS?

Will be built in OfBorg automatically, but I will review/build before merging.

Which NIX_* variables do we need specifically?

At least NIX_LD_FLAGS and NIX_COMPILE_FLAGS, please take a look at issue oracle/graal#7502.

But actually there are many more flags that needs to be passed, this is why we decided by simply using NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION for now.

You can take a look at the derivation that we pass to the builder, and pretty much every flag with NIX_* need to be passed because they will ensure we are building correctly (for example, I know we pass an environment variable to ensure reproducibility by setting a seed for random.h, not sure the name of the environment variable right now).

If it's only NIX_LD_FLAGS we can pass that along using -ENIX_LD_FLAGS in nativeImageBuildArgs.

So that is the point of the issue above, we need to have a pretty good understanding of everything we need to pass to the builder environment to make sure that we are not missing anything.


BTW, why is babashka failing now that they're explicitly passing the environment variables to the builder? Does the builder fail when we set NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION and use the -E flag? Can you post the error that we get?

@thiagokokada
Copy link
Contributor

Also, please take a look at #273961 to understand the context why we are using NATIVE_IMAGE_DEPRECATED_BUILDER_SANITAITON and the implications of removing it.

@Sohalt
Copy link
Contributor Author

Sohalt commented Jan 22, 2024

BTW, why is babashka failing now that they're explicitly passing the environment variables to the builder? Does the builder fail when we set NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION and use the -E flag? Can you post the error that we get?

Yes, babashka is failing now, since NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION is incompatible with the -E flag, which is the whole reason I started this PR:

Running phase: buildPhase
Apply jar:file:///nix/store/xk91l27wxfh9bb16r5wkq2cvgd7qdvwf-babashka-1.3.188-standalone.jar!/META-INF/native-image/clj-commons/clj-yaml/native-image.properties
Apply jar:file:///nix/store/xk91l27wxfh9bb16r5wkq2cvgd7qdvwf-babashka-1.3.188-standalone.jar!/META-INF/native-image/http-kit/http-kit/native-image.properties
Apply jar:file:///nix/store/xk91l27wxfh9bb16r5wkq2cvgd7qdvwf-babashka-1.3.188-standalone.jar!/META-INF/native-image/babashka/babashka/native-image.properties
Warning: Ignoring server-mode native-image argument --no-server.
Apply jar:file:///nix/store/xk91l27wxfh9bb16r5wkq2cvgd7qdvwf-babashka-1.3.188-standalone.jar!/META-INF/native-image/babashka/babashka/reflect-config.json
Warning: The option '-H:+JNI' is experimental and must be enabled via '-H:+UnlockExperimentalVMOptions' in the future.
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=javax.sound.sampled.spi.AudioFileWriter' is experimental and must be enabled via '-H:+UnlockExperime>
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=java.nio.charset.spi.CharsetProvider' is experimental and must be enabled via '-H:+UnlockExperimenta>
Warning: The option '-H:IncludeResources=SCI_VERSION' is experimental and must be enabled via '-H:+UnlockExperimentalVMOptions' in the future.
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=javax.sound.midi.spi.MidiDeviceProvider' is experimental and must be enabled via '-H:+UnlockExperime>
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=javax.sound.midi.spi.SoundbankReader' is experimental and must be enabled via '-H:+UnlockExperimenta>
Warning: The option '-H:Name=bb' is experimental and must be enabled via '-H:+UnlockExperimentalVMOptions' in the future.
Warning: The option '-H:IncludeResources=META-INF/babashka/.*' is experimental and must be enabled via '-H:+UnlockExperimentalVMOptions' in the future.
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=javax.sound.midi.spi.MidiFileWriter' is experimental and must be enabled via '-H:+UnlockExperimental>
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=java.net.ContentHandlerFactory' is experimental and must be enabled via '-H:+UnlockExperimentalVMOpt>
Warning: The option '-H:IncludeResources=BABASHKA_VERSION' is experimental and must be enabled via '-H:+UnlockExperimentalVMOptions' in the future.
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=javax.sound.midi.spi.MidiFileReader' is experimental and must be enabled via '-H:+UnlockExperimental>
Warning: The option '-H:IncludeResources=src/babashka/.*' is experimental and must be enabled via '-H:+UnlockExperimentalVMOptions' in the future.
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=javax.sound.sampled.spi.AudioFileReader' is experimental and must be enabled via '-H:+UnlockExperime>
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=javax.sound.sampled.spi.FormatConversionProvider' is experimental and must be enabled via '-H:+Unloc>
Warning: The option '-H:Log=registerResource:3' is experimental and must be enabled via '-H:+UnlockExperimentalVMOptions' in the future.
Warning: The option '-H:ServiceLoaderFeatureExcludeServices=javax.sound.sampled.spi.MixerProvider' is experimental and must be enabled via '-H:+UnlockExperiment>
Warning: Please re-evaluate whether any experimental option is required, and either remove or unlock it. The build output lists all active experimental options,>
Apply jar:file:///nix/store/r3nnadqvkaxw2m4787gmmr72ydmwfqh2-graalvm-ce-21.0.1/lib/svm/library-support.jar!/META-INF/native-image/com.oracle.svm/thirdparty/nati>
Apply jar:file:///nix/store/r3nnadqvkaxw2m4787gmmr72ydmwfqh2-graalvm-ce-21.0.1/lib/svm/library-support.jar!/META-INF/native-image/com.oracle.svm/polyglot/native>
Error: Option -E<env-var-key>[=<env-var-value>] is not compatible with environment variable NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION=true.
com.oracle.svm.driver.NativeImage$NativeImageError: Option -E<env-var-key>[=<env-var-value>] is not compatible with environment variable NATIVE_IMAGE_DEPRECATED>
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.showError(NativeImage.java:2328)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.buildImage(NativeImage.java:1666)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.completeImageBuild(NativeImage.java:1290)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.build(NativeImage.java:1913)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.performBuild(NativeImage.java:1878)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.main(NativeImage.java:1852)
        at java.base@21.0.1/java.lang.invoke.LambdaForm$DMH/sa346b79c.invokeStaticInit(LambdaForm$DMH)

I could probably do something like overrideArgs to just disable NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION in buildNativeImage used for babashka, but that also seems very hacky.

@thiagokokada
Copy link
Contributor

I could probably do something like overrideArgs to just disable NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION in buildNativeImage used for babashka, but that also seems very hacky.

This will not solve the issue since babashka will fail to build in darwin.

@Sohalt
Copy link
Contributor Author

Sohalt commented Jan 22, 2024

What about constructing the args to native-image dynamically from a call to env? Then we'd have the same behavior as currently, but using the -E flags.

@thiagokokada
Copy link
Contributor

What about constructing the args to native-image dynamically from a call to env? Then we'd have the same behavior as currently, but using the -E flags.

Yes, this would work. Are you interested in working on this @Sohalt ?

Pass the whole environment to the native-image build process by
generating a -E option for every environment variable.

This has the same effect as setting
NATIVE_IMAGE_DEPRECATED_BUILDER_SANITATION=true
but is compatible with packages providing -E options themselves
@thiagokokada
Copy link
Contributor

Result of nixpkgs-review pr 282901 run on aarch64-darwin 1

1 package marked as broken and skipped:
  • obb
1 package failed to build:
  • scala-update
10 packages built:
  • babashka
  • babashka-unwrapped
  • bbin
  • clj-kondo
  • cljfmt
  • clojure-lsp
  • jet
  • neil
  • vscode-extensions.betterthantomorrow.calva
  • zprint

@thiagokokada thiagokokada merged commit df318b8 into NixOS:master Jan 22, 2024
22 of 23 checks passed
@thiagokokada
Copy link
Contributor

Thanks @Sohalt.

@Sohalt Sohalt deleted the update-babashka branch January 23, 2024 09:16
thiagokokada added a commit to thiagokokada/nixpkgs that referenced this pull request Jan 26, 2024
Now that we can pass build arguments for the GraalVM builder again (see
NixOS#282901), this should work again.

Fix issue: NixOS#283953
thiagokokada added a commit to thiagokokada/nixpkgs that referenced this pull request Jan 26, 2024
Now that we can pass build arguments for the GraalVM builder again (see
NixOS#282901), this should work again.

Fix issue: NixOS#283953
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants