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

pkl: init at 0.26.2 #286658

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

pkl: init at 0.26.2 #286658

wants to merge 1 commit into from

Conversation

rafaelrc7
Copy link
Member

@rafaelrc7 rafaelrc7 commented Feb 6, 2024

Description of changes

A configuration as code language with rich validation and tooling.

Homepage: https://pkl-lang.org/
Source: https://github.com/apple/pkl/

Closes #286104 .

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.

Add a 👍 reaction to pull requests you find important.

@rafaelrc7
Copy link
Member Author

This PR is a draft because I'm running into some difficulties to package it. I solved lots of errors, but the program still not builds. Currently it errors with:

<=============> 100% CONFIGURING [47s]> IDLELoaded lock state for configuration ':pkl-cli:runtimeClasspath'

FAILURE: Build failed with an exception.

* What went wrong:
Could not determine the dependencies of task ':pkl-cli:shadowJar'.
> Could not resolve all dependencies for configuration ':pkl-cli:runtimeClasspath'.
   > Did not resolve 'com.github.ajalt.clikt:clikt-jvm:3.5.1' which is part of the dependency lock state

The code seems to be downloading this dependency. The problem seems to be that this task, for some reason, is not using the local repo.

@lilyball
Copy link
Member

lilyball commented Feb 8, 2024

According to the Pkl installation instructions, the java executable has a noticeable startup delay and runs complex code slower than the native executables. It would be great if this derivation could produce the native executables for supported platforms instead.

@rafaelrc7
Copy link
Member Author

It would be great if this derivation could produce the native executables for supported platforms instead.

Indeed it would... However, the native executable depends on the java build process, so I am trying to fix it first. And the native build also depends on GraalVM for JDK 11, that seems to be a problem, as it was removed from nixpkgs some months ago because it got unsupported. So I'm not sure how to deal with that. When I tried building with the graalvm package currently available (for JDK 21), it did not work, maybe it is fixable, but, again, I'm having trouble with the java build, as described above.

@lilyball
Copy link
Member

lilyball commented Feb 8, 2024

I'd also love to see support for pkldoc as its own executable, which currently requires invoking a Jar bundled with the Java library.

@rafaelrc7
Copy link
Member Author

I also asked about the gradle issues in the pkl repo apple/pkl#90 . I'm waiting for suggestions either here or there. Also trying some stuff by myself, but I did not have much luck.

@lilyball
Copy link
Member

lilyball commented Mar 6, 2024

Last week the pkl repo issue was updated with more information

@lilyball
Copy link
Member

Pkl 0.26.0 has been released.

@chayleaf
Copy link
Contributor

#272380 got merged, so you can give this a second shot now! See https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/gradle.section.md

@rafaelrc7 rafaelrc7 changed the title pkl: init at 0.25.1 pkl: init at 0.26.1 Jul 19, 2024
@rafaelrc7
Copy link
Member Author

Thanks for the work, @chayleaf ! It really helped with gradle packages and eased a lot of the verbosity.

However, this package still seems to face the same problems... I've done as you tell in the manual page you wrote, and rewrote the package.nix following the new model. Then, I ran $(nix-build -A pkl.mitmCache.updateScript), that worked and generated the dependency file that I just pushed. However, when I tried to build the package, it seems not all dependencies are being pulled, as it still failed:

error: builder for '/nix/store/sag7gkf9qlb9ym4f7vb64r0bfncm4rv0-pkl-0.26.1.drv' failed with exit code 1;
       last 25 log lines:
       > Could not determine the dependencies of task ':bench:spotlessCheck'.
       > > Could not create task ':bench:spotlessJavaCheck'.
       >    > Could not create task ':bench:spotlessJava'.
       >       > Could not resolve all files for configuration ':bench:spotless865481289'.
       >          > Could not find com.google.googlejavaformat:google-java-format:1.21.0.
       >            Searched in the following locations:
       >              - https://repo.maven.apache.org/maven2/com/google/googlejavaformat/google-java-format/1.21.0/google-java-format-1.21.0.pom
       >            Required by:
       >                project :bench
       >
       > * Try:
       > > If the artifact you are trying to retrieve can be found in the repository but without metadata in 'Maven POM' format, you need to adjust the 'metadataSources { ... }' of the repository declaration.
       > > Run with --stacktrace option to get the stack trace.
       > > Run with --info or --debug option to get more log output.
       > > Run with --scan to get full insights.
       > > Get more help at https://help.gradle.org.
       >
       > Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
       >
       > You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
       >
       > For more on this, please refer to https://docs.gradle.org/8.8/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.
       >
       > BUILD FAILED in 1m 9s
       > 10 actionable tasks: 10 executed
       For full logs, run 'nix log /nix/store/sag7gkf9qlb9ym4f7vb64r0bfncm4rv0-pkl-0.26.1.drv'.

And this dependency (com.google.googlejavaformat:google-java-format:1.21.0) is indeed missing from the deps.json file.

As a test, I tried removing this dependency from the project, by adding:

  configurePhase = ''
    sed -ie "s#googleJavaFormat(libs.versions.googleJavaFormat.get())# #g" ./buildSrc/src/main/kotlin/pklJavaLibrary.gradle.kts
  '';

to the derivation. However, this resulted in another error:

error: builder for '/nix/store/p184jzg1xx1hxnwzzajydd33k9b2qv0c-pkl-0.26.1.drv' failed with exit code 1;
       last 25 log lines:
       > FAILURE: Build failed with an exception.
       >
       > * Where:
       > Build file '/build/source/buildSrc/build.gradle.kts' line: 3
       >
       > * What went wrong:
       > Plugin [id: 'org.gradle.kotlin.kotlin-dsl', version: '4.3.1'] was not found in any of the following sources:
       >
       > - Gradle Core Plugins (plugin is not in 'org.gradle' namespace)
       > - Included Builds (No included builds contain this plugin)
       > - Plugin Repositories (could not resolve plugin artifact 'org.gradle.kotlin.kotlin-dsl:org.gradle.kotlin.kotlin-dsl.gradle.plugin:4.3.1')
       >   Searched in the following repositories:
       >     MavenRepo
       >     Gradle Central Plugin Repository
       >
       > * Try:
       > > Run with --stacktrace option to get the stack trace.
       > > Run with --info or --debug option to get more log output.
       > > Run with --scan to get full insights.
       > > Get more help at https://help.gradle.org.
       >
       > BUILD FAILED in 14s
       > 
       > 
       > <-------------> 0% WAITING> IDLE
       For full logs, run 'nix log /nix/store/p184jzg1xx1hxnwzzajydd33k9b2qv0c-pkl-0.26.1.drv'.

This one, however, seems to have been add to the dependency file:

  "org/gradle/kotlin#gradle-kotlin-dsl-plugins/4.3.1": {
   "jar": "sha256-j8X5xgrEWoeaQXdoEbWf0s5cWtXkJW8S18i+rFCkHcg=",
   "module": "sha256-k+eJyXjl4QG8QXfLsnmFQSvMtpNtsA7MQeYoG4QD/F4=",
   "pom": "sha256-Fr3gZfWofPRP3FD5xbYNMS9xOgwbyq4AixJItHhojAo="
  },
  "org/gradle/kotlin/kotlin-dsl#org.gradle.kotlin.kotlin-dsl.gradle.plugin/4.3.1": {
   "pom": "sha256-Oispc8Afy2j0kx8ZbNeLAzctoWrBYmqOPWMYVkNqlOc="
  },

So I am not sure what the issue is here... I notice the second item has the pom hash, but not the jar hash, is this related?

@chayleaf
Copy link
Contributor

chayleaf commented Jul 19, 2024

This is using spotless, a plugin that's using dynamic dependencies for code formatting/etc, which makes no sense for a normal build. You should disable it using gradleFlags = [ "-x" "spotlessCheck" ]; (like stirling-pdf).

@chayleaf
Copy link
Contributor

chayleaf commented Jul 19, 2024

Fixed the build:

diff --git a/pkgs/by-name/pk/pkl/package.nix b/pkgs/by-name/pk/pkl/package.nix
index 1142728572e0..3c8a8a1e4e32 100644
--- a/pkgs/by-name/pk/pkl/package.nix
+++ b/pkgs/by-name/pk/pkl/package.nix
@@ -14,13 +14,24 @@ stdenv.mkDerivation rec {
     owner = "apple";
     repo = "pkl";
     rev = version;
-    sha256 = "sha256-9YXccQloPxGekkR78tvkWKCmB9wXzL7iFkRL4yLQVZQ=";
+    sha256 = "sha256-Q7B6DRKmgysba+VhvKiTE98UA52i6UUfsvk3Tl/2Rqg=";
+    # the build needs the commit id, replace it in postFetch and remove .git manually
+    leaveDotGit = true;
+    postFetch = ''
+      cd "$out"
+      export commit_id=$(git rev-parse --short HEAD)
+      cp ${./set_commit_id.patch} set_commit_id.patch
+      chmod +w set_commit_id.patch
+      substituteAllInPlace set_commit_id.patch
+      git apply set_commit_id.patch
+      rm set_commit_id.patch
+      find "$out" -name .git -print0 | xargs -0 rm -rf
+    '';
   };
 
   nativeBuildInputs = [
     git
     gradle
-    jdk17
   ];
 
   buildInputs = [
@@ -34,9 +45,13 @@ stdenv.mkDerivation rec {
 
   __darwinAllowLocalNetworking = true;
 
-  gradleFlags = [ "-Dfile.encoding=utf-8" ];
+  gradleFlags = [
+    "-x" "spotlessCheck"
+    "-DreleaseBuild=true"
+    "-Dorg.gradle.java.home=${jdk17}"
+  ];
 
-  gradleBuildTask = "build";
+  JAVA_TOOL_OPTIONS = "-Dfile.encoding=utf-8";
 
   doCheck = false;
 
@@ -52,6 +67,10 @@ stdenv.mkDerivation rec {
     platforms = platforms.all;
     maintainers = with maintainers; [ rafaelrc ];
     mainProgram = "pkl";
+    sourceProvenance = with lib.sourceTypes; [
+      fromSource
+      binaryBytecode # mitm cache
+    ];
   };
 }
 
diff --git a/pkgs/by-name/pk/pkl/set_commit_id.patch b/pkgs/by-name/pk/pkl/set_commit_id.patch
new file mode 100644
index 000000000000..081a523c579a
--- /dev/null
+++ b/pkgs/by-name/pk/pkl/set_commit_id.patch
@@ -0,0 +1,20 @@
+diff --git a/buildSrc/src/main/kotlin/BuildInfo.kt b/buildSrc/src/main/kotlin/BuildInfo.kt
+index 0264375..dfabed1 100644
+--- a/buildSrc/src/main/kotlin/BuildInfo.kt
++++ b/buildSrc/src/main/kotlin/BuildInfo.kt
+@@ -87,14 +87,7 @@ open class BuildInfo(project: Project) {
+   val commitId: String by lazy {
+     // only run command once per build invocation
+     if (project === project.rootProject) {
+-      val process = ProcessBuilder()
+-        .command("git", "rev-parse", "--short", "HEAD")
+-        .directory(project.rootDir)
+-        .start()
+-      process.waitFor().also { exitCode ->
+-        if (exitCode == -1) throw RuntimeException(process.errorStream.reader().readText())
+-      }
+-      process.inputStream.reader().readText().trim()
++      "@commit_id@"
+     } else {
+       project.rootProject.extensions.getByType(BuildInfo::class.java).commitId
+     }

You will also need to patch the binary so it points to the correct jre.

The tests might require further workarounds to run though.

@chayleaf
Copy link
Contributor

and after this patch it works fine

diff --git a/pkgs/by-name/pk/pkl/package.nix b/pkgs/by-name/pk/pkl/package.nix
index 3c8a8a1e4e32..a871a1574759 100644
--- a/pkgs/by-name/pk/pkl/package.nix
+++ b/pkgs/by-name/pk/pkl/package.nix
@@ -3,7 +3,6 @@
 , gradle
 , lib
 , fetchFromGitHub
-, jre
 , git
 }:
 stdenv.mkDerivation rec {
@@ -34,10 +33,6 @@ stdenv.mkDerivation rec {
     gradle
   ];
 
-  buildInputs = [
-    jre
-  ];
-
   mitmCache = gradle.fetchDeps {
     inherit pname;
     data = ./deps.json;
@@ -57,7 +52,9 @@ stdenv.mkDerivation rec {
 
   installPhase = ''
     mkdir -p "$out/bin"
-    install -Dm755 "./pkl-cli/build/executable/jpkl" "$out/bin/pkl"
+    head -n2 ./pkl-cli/build/executable/jpkl | sed 's%java%${jdk17}/bin/java%' > "$out/bin/pkl"
+    tail -n+3 ./pkl-cli/build/executable/jpkl >> "$out/bin/pkl"
+    chmod 755 "$out/bin/pkl"
   '';
 
   meta = with lib; {

@rafaelrc7
Copy link
Member Author

@chayleaf Oh, I see you were also working on this, thanks!

I managed to build it using the pkl-cli:javaExecutable target. The commit I just pushed works and builds successfully. I also patched the source code to generate the executable with the proper java path, instead of patching the executable after the fact. But I understand you managed to make the whole build work? I will take a look into your changes.

@chayleaf
Copy link
Contributor

set_commit_id.patch and postFetch is necessary because one of the tests checks that the commit ID is set. -x spotlessCheck is useful too if you ever want to get tests to work. I'm not setting gradleBuildTask at all so it defaults to assemble. And JAVA_TOOL_OPTIONS = "-Dfile.encoding=utf-8"; was necessary for javadoc generation, which granted may not be included in the final jar.

@rafaelrc7
Copy link
Member Author

I noticed you added jdk17 back. I managed to build it and run it without it. I did not add any jdk to the javahome flags, and used jre to set the java binary in the final executable, and it seems to be running fine. Pkl's docs state that you need JDK 17 or higher. Is there any other reason for you using it?

@chayleaf
Copy link
Contributor

Since the target jre version is 17 (see https://github.com/apple/pkl/blob/0.26.2/buildSrc/build.gradle.kts), it makes sense to build using jdk17 - that's the only reason.

@chayleaf
Copy link
Contributor

chayleaf commented Jul 20, 2024

oh yeah, just to clarify - the test checks that the language exports the commit ID of pkl, that's why it's necessary to set it (of course, the alternative is to leave .git and add git to native build inputs)

@rafaelrc7 rafaelrc7 changed the title pkl: init at 0.26.1 pkl: init at 0.26.2 Jul 20, 2024
@rafaelrc7
Copy link
Member Author

Thanks for your help, @chayleaf !

The package is building and is usable now, so I believe this PR is ready.

@lilyball There is still one final issue left, the native build. Last time I looked into it, it depended on a specific GraalVM version that was removed from nixpkgs because it was EOL. So I expected it to be an issue when the base java build was finally fixed. I will start looking into it again, but in a different branch and mark this one as ready for review, because of the already great delay.

@rafaelrc7 rafaelrc7 marked this pull request as ready for review July 20, 2024 00:50
Co-authored-by: chayleaf <[email protected]>
@rafaelrc7
Copy link
Member Author

rafaelrc7 commented Jul 20, 2024

@lilyball I started the native build work here: https://github.com/rafaelrc7/nixpkgs/blob/pkl/native/pkgs/by-name/pk/pkl/package.nix . If you want to help. Currently it is not building, I plan on creating a new PR after this one is merged. I believe the problems come from the fact that pkl seems to depend on an older version of graalvm (17). I changed a flag from macro:truffle to macro:truffle-svm, but it fails with: Fatal error: java.lang.module.FindException: Module org.graalvm.polyglot not found, required by org.graalvm.truffle.runtime.svm, it seems that the polyglot dependency is missing?

@chayleaf
Copy link
Contributor

chayleaf commented Jul 24, 2024

I fixed the tests, but this requires #329554 (edit: it got merged)

diff
diff --git a/pkgs/by-name/pk/pkl/disable_gradle_codegen_tests.patch b/pkgs/by-name/pk/pkl/disable_gradle_codegen_tests.patch
new file mode 100644
index 000000000000..b3e9d78970cf
--- /dev/null
+++ b/pkgs/by-name/pk/pkl/disable_gradle_codegen_tests.patch
@@ -0,0 +1,68 @@
+diff --git a/pkl-gradle/src/test/kotlin/org/pkl/gradle/JavaCodeGeneratorsTest.kt b/pkl-gradle/src/test/kotlin/org/pkl/gradle/JavaCodeGeneratorsTest.kt
+index 277d6c2..254477d 100644
+--- a/pkl-gradle/src/test/kotlin/org/pkl/gradle/JavaCodeGeneratorsTest.kt
++++ b/pkl-gradle/src/test/kotlin/org/pkl/gradle/JavaCodeGeneratorsTest.kt
+@@ -1,11 +1,13 @@
+ package org.pkl.gradle
+ 
+ import org.assertj.core.api.Assertions.assertThat
++import org.junit.jupiter.api.Disabled;
+ import org.junit.jupiter.api.Test
+ import kotlin.io.path.listDirectoryEntries
+ import kotlin.io.path.readText
+ 
+ class JavaCodeGeneratorsTest : AbstractTest() {
++  @Disabled("runTask doesn't use the MITM cache")
+   @Test
+   fun `generate code`() {
+     writeBuildFile()
+@@ -50,6 +52,7 @@ class JavaCodeGeneratorsTest : AbstractTest() {
+     )
+   }
+ 
++  @Disabled("runTask doesn't use the MITM cache")
+   @Test
+   fun `compile generated code`() {
+     writeBuildFile()
+@@ -66,6 +69,7 @@ class JavaCodeGeneratorsTest : AbstractTest() {
+     assertThat(addressClassFile).exists()
+   }
+   
++  @Disabled("runTask doesn't use the MITM cache")
+   @Test
+   fun `no source modules`() {
+     writeFile(
+diff --git a/pkl-gradle/src/test/kotlin/org/pkl/gradle/KotlinCodeGeneratorsTest.kt b/pkl-gradle/src/test/kotlin/org/pkl/gradle/KotlinCodeGeneratorsTest.kt
+index 8ede4c6..93578b0 100644
+--- a/pkl-gradle/src/test/kotlin/org/pkl/gradle/KotlinCodeGeneratorsTest.kt
++++ b/pkl-gradle/src/test/kotlin/org/pkl/gradle/KotlinCodeGeneratorsTest.kt
+@@ -1,11 +1,13 @@
+ package org.pkl.gradle
+ 
+ import org.assertj.core.api.Assertions.assertThat
++import org.junit.jupiter.api.Disabled;
+ import org.junit.jupiter.api.Test
+ import kotlin.io.path.listDirectoryEntries
+ import kotlin.io.path.readText
+ 
+ class KotlinCodeGeneratorsTest : AbstractTest() {
++  @Disabled("runTask doesn't use the MITM cache")
+   @Test
+   fun `generate code`() {
+     writeBuildFile()
+@@ -51,6 +53,7 @@ class KotlinCodeGeneratorsTest : AbstractTest() {
+     )
+   }
+ 
++  @Disabled("runTask doesn't use the MITM cache")
+   @Test
+   fun `compile generated code`() {
+     writeBuildFile()
+@@ -66,6 +69,7 @@ class KotlinCodeGeneratorsTest : AbstractTest() {
+     assertThat(addressClassFile).exists()
+   }
+   
++  @Disabled("runTask doesn't use the MITM cache")
+   @Test
+   fun `no source modules`() {
+     writeFile(
diff --git a/pkgs/by-name/pk/pkl/fix_kotlin_classpath.patch b/pkgs/by-name/pk/pkl/fix_kotlin_classpath.patch
new file mode 100644
index 000000000000..5b28c800adfd
--- /dev/null
+++ b/pkgs/by-name/pk/pkl/fix_kotlin_classpath.patch
@@ -0,0 +1,13 @@
+diff --git a/pkl-gradle/pkl-gradle.gradle.kts b/pkl-gradle/pkl-gradle.gradle.kts
+index 55a03fc..8968dff 100644
+--- a/pkl-gradle/pkl-gradle.gradle.kts
++++ b/pkl-gradle/pkl-gradle.gradle.kts
+@@ -40,7 +40,7 @@ sourceSets {
+   test {
+     // Remove Gradle distribution JARs from test compile classpath.
+     // This prevents a conflict between Gradle's and Pkl's Kotlin versions.
+-    compileClasspath = compileClasspath.filter { !(it.path.contains("dists")) }
++    compileClasspath = compileClasspath.filter { !(it.path.contains("@gradle@") || it.path.contains("generated-gradle-jars/gradle-api-")) }
+   }
+ }
+ 
diff --git a/pkgs/by-name/pk/pkl/package.nix b/pkgs/by-name/pk/pkl/package.nix
index 273a7ec8f80b..d5fa99285c1c 100644
--- a/pkgs/by-name/pk/pkl/package.nix
+++ b/pkgs/by-name/pk/pkl/package.nix
@@ -1,5 +1,6 @@
 { stdenv
 , lib
+, substituteAll
 , fetchFromGitHub
 , gradle
 , jdk17
@@ -27,6 +28,14 @@ stdenv.mkDerivation rec {
     '';
   };
 
+  patches = [
+    (substituteAll {
+      src = ./fix_kotlin_classpath.patch;
+      gradle = gradle.unwrapped;
+    })
+    ./disable_gradle_codegen_tests.patch
+  ];
+
   nativeBuildInputs = [ gradle ];
 
   mitmCache = gradle.fetchDeps {
@@ -42,6 +51,8 @@ stdenv.mkDerivation rec {
     "-Dorg.gradle.java.home=${jdk17}"
   ];
 
+  doCheck = true;
+
   JAVA_TOOL_OPTIONS = "-Dfile.encoding=utf-8";
 
   installPhase = ''

@rafaelrc7 rafaelrc7 mentioned this pull request Jul 25, 2024
13 tasks
@StayBlue
Copy link

StayBlue commented Aug 9, 2024

There's a new release of pkl with version 0.26.3.

@dakota-doordash
Copy link
Contributor

Hi! Is there any update on this? Anything blocking this from going through?

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.

Package request: pkl
5 participants