-
Notifications
You must be signed in to change notification settings - Fork 280
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
Upstream: Build Upgrades #204
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review/tour below.
- name: "🛠️ Dependency Graph" | ||
uses: gradle/actions/dependency-submission@417ae3ccd767c252f5661f1ace9f835f9654f2b5 # v3.1.0 | ||
continue-on-error: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builds and submits the Gradle dependency graph to GitHub, so it can be used for vulnerability checks and Pkl can show up in repository used-by/uses listings.
bench/bench.gradle.kts
Outdated
jmh(project(":pkl-core")) | ||
jmh(projects.pklCore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gradle's typed project accessors are the equivalent of Version Catalogs but for project dependencies.
jmhVersion.set(libs.versions.jmh) | ||
jmhVersion = libs.versions.jmh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In newer versions of Gradle's Kotlin DSL, there is no need to call .set(...)
on properties; a compiler plugin takes care of the assignment syntax.
build.gradle.kts
Outdated
).forEach { | ||
kover(it) | ||
testReportAggregation(it) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All projects with tests were added to merged test and coverage reporting.
build.gradle.kts
Outdated
val reports by tasks.registering { | ||
description = "Generates all reports" | ||
group = "Reporting" | ||
|
||
dependsOn(tasks.named("allTestsReport")) | ||
} | ||
|
||
val check by tasks.registering { | ||
description = "Runs all checks" | ||
group = "Verification" | ||
|
||
finalizedBy( | ||
reports, | ||
coverageReports, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gradlew test check
builds and checks and reports on all the things
gradle.properties
Outdated
# bytecode targeting & compiler settings | ||
strict=false | ||
javaTarget=11 | ||
javaEntrypointTarget=11 | ||
javaToolchainTarget=21 | ||
kotlinTarget=1.9 | ||
kotlinBeta=false | ||
|
||
# build settings | ||
lockDependencies=false | ||
xmlReporting=false | ||
sarifReporting=false | ||
htmlReporting=false | ||
autofixDetekt=false | ||
remoteCache=false | ||
cachePush=false | ||
|
||
# kotlin settings | ||
kotlin.stdlib.default.dependency=false | ||
|
||
# gradle settings | ||
org.gradle.caching=true | ||
org.gradle.configuration-cache=false | ||
org.gradle.configuration-cache-problems=warn | ||
org.gradle.dependency-verification=lenient | ||
org.gradle.parallel=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjustable build settings in the root gradle.properties
. Dependency Verification is left in LENIENT
mode for now so that configurations can be built and then sealed.
public final class org/pkl/config/kotlin/ConfigExtensionsKt { | ||
public static final fun forKotlin (Lorg/pkl/config/java/ConfigEvaluator;)Lorg/pkl/config/java/ConfigEvaluator; | ||
public static final fun forKotlin (Lorg/pkl/config/java/ConfigEvaluatorBuilder;)Lorg/pkl/config/java/ConfigEvaluatorBuilder; | ||
public static final fun forKotlin (Lorg/pkl/config/java/mapper/ValueMapperBuilder;)Lorg/pkl/config/java/mapper/ValueMapperBuilder; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These API pins will ensure that applicable modules (published publicly) have a known interface. Breakages can be checked before merging PRs, to ensure downstream library consumers aren't broken by a given change.
The plugin that produces this is made by JetBrains (available here). It is used for core libraries like KotlinX Serialization, etc, so it's very reliable and well maintained.
Update pins: ./gradlew apiDump
Check pins: ./gradlew apiCheck
buildConfig { | ||
sourceSets { | ||
named("test") { | ||
packageName("org.pkl.gradle.constants.test") | ||
useKotlinOutput { topLevelConstants = true } | ||
|
||
buildConfigField("KOTLIN_VERSION", libs.versions.kotlin) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Gradle tests now pin to the same Kotlin version as the build. A symbol, KOTLIN_VERSION
, is generated within the tests, which is used in the embedded Gradle build.
settings.gradle.kts
Outdated
plugins { | ||
id("build.less") version "1.0.0-rc2" | ||
id("com.gradle.enterprise") version "3.16.2" | ||
id("org.gradle.toolchains.foojay-resolver-convention") version "0.8.0" | ||
id("com.gradle.common-custom-user-data-gradle-plugin") version "1.12.1" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what's going on here:
- Buildless enhances build caching support
- Gradle Enterprise allows build scans with
--scan
- Foojay Resolver dynamically provisions Java toolchains
- Common User Data plugin enhances build scans
settings.gradle.kts
Outdated
buildless { | ||
remoteCache { | ||
enabled = extra.properties["remoteCache"] == "true" | ||
push.set(extra.properties["cachePush"] == "true") | ||
} | ||
} | ||
|
||
buildCache { | ||
local { | ||
isEnabled = true | ||
removeUnusedEntriesAfterDays = 14 | ||
directory = file(".codebase/build-cache") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remote caching is off by default; local caching cleans itself every 14 days and stores local artifacts in .codebase/build-cache
. Thus, doing a full clean/reset/clone etc. will drop your build cache, too.
I intend to benchmark this against Other material with data about this change:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the stuff in here is looking very promising and highly desirable, so thanks!
I would, however, suggest PR-ing at a finer granularity. Version-bumping the dependencies, for example, is high-noise, low signal; typically best done as a separate PR.
// Configurations which are subject to dependency locking. | ||
private val lockedConfigurations = listOf( | ||
"annotationProcessor", | ||
"archives", | ||
"compileClasspath", | ||
"graal", | ||
"kotlinCompilerClasspath", | ||
"kotlinKlibCommonizerClasspath", | ||
"runtimeClasspath", | ||
"testCompileClasspath", | ||
"testRuntimeClasspath", | ||
"truffle", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smaller PRs would definitely help smoother review+merge processes. This is not at all to discourage you from the lovely work you're doing!
@holzensp You are totally right! The intent is not to merge this PR, but instead to offer a menu of changes for the Pkl team from our fork. I am completely happy to split any of these enclosed changes up into their own PRs. Edit: It seems to me there are some very sensible changes I can split off first and file separately, for example, the Version Catalog tool fix. I'll clean up my existing PRs and file that first couple for review. |
- fix: make version catalog accessible from `buildSrc` plugins - chore: declare `googleFormatVersion` in version catalog - chore: declare `ktfmt` in version catalog Relates-To: apple#204 Signed-off-by: Sam Gammon <[email protected]>
This change activates the `TYPESAFE_PROJECT_ACCESSORS` feature preview in Gradle, and switches to such accessors instead of string-based project references, where possible Relates-To: apple#204 Signed-off-by: Sam Gammon <[email protected]>
Okay, I will pause there for now because I'm not sure which of these other changes you guys would like to see filed. Liking comments from the self-tour is a good way to let me know. The first three are pretty reasonable and don't touch lockfiles at all, so we can avoid conflicts and I can make my lockfile changes more specific. |
4c0b0a8
to
8e087be
Compare
@@ -108,8 +108,9 @@ open class BuildInfo(project: Project) { | |||
val commitId: String by lazy { | |||
// only run command once per build invocation | |||
if (project === project.rootProject) { | |||
Runtime.getRuntime() | |||
.exec("git rev-parse --short HEAD", arrayOf(), project.rootDir) | |||
ProcessBuilder("git", "rev-parse", "--short", "HEAD") |
Check failure
Code scanning / CodeQL
Executing a command with a relative path
8e087be
to
3c9c2a1
Compare
This change activates the `TYPESAFE_PROJECT_ACCESSORS` feature preview in Gradle, and switches to such accessors instead of string-based project references, where possible Relates-To: apple#204 Signed-off-by: Sam Gammon <[email protected]>
- fix: make version catalog accessible from `buildSrc` plugins - chore: declare `googleFormatVersion` in version catalog - chore: declare `ktfmt` in version catalog Relates-To: apple#204 Signed-off-by: Sam Gammon <[email protected]>
This change activates the `TYPESAFE_PROJECT_ACCESSORS` feature preview in Gradle, and switches to such accessors instead of string-based project references, where possible Relates-To: #204 Signed-off-by: Sam Gammon <[email protected]>
- fix: make version catalog accessible from `buildSrc` plugins - chore: declare `googleFormatVersion` in version catalog - chore: declare `ktfmt` in version catalog Relates-To: #204 Signed-off-by: Sam Gammon <[email protected]>
32527fd
to
cf6dc88
Compare
/** | ||
* @return Whether the provided logging level is enabled; defaults to `false` | ||
*/ | ||
default boolean isLevelEnabled(Level level) { |
Check notice
Code scanning / CodeQL
Useless parameter
|
||
/** Returns a logger that sends log messages to the logger at the provided name. */ | ||
@SuppressWarnings("DuplicatedCode") | ||
public static Logger logger(String name) { |
Check notice
Code scanning / CodeQL
Useless parameter
* @param configuration Configuration context to determine eligibility | ||
* @return Whether the plug-in should run; defaults to `true`. | ||
*/ | ||
default Boolean isInConfiguration(PklPluginConfiguration configuration) { |
Check notice
Code scanning / CodeQL
Useless parameter
/** Returns a logger that sends log messages to the logger at the provided name. */ | ||
@SuppressWarnings("DuplicatedCode") | ||
public static Logger logger(String name) { | ||
var target = stdErr(); |
Check notice
Code scanning / CodeQL
Unread local variable
/** Built-in plugin which tunes the VM execution context. */ | ||
@SuppressWarnings("unused") | ||
public class VmEnginePlugin implements PklPlugin { | ||
class VmContextListener implements ContextEventListener { |
Check notice
Code scanning / CodeQL
Inner class could be static
72e31b6
to
7bd65d4
Compare
Fixes and closes apple#191 Signed-off-by: Sam Gammon <[email protected]>
Adds a setting to the Kotlin code generator which controls the target Kotlin package for codegen. Applied as a prefix to the normal generated package name. The current versions of the `kotlinx.serialization` and `kotlinx.html` libraries are declared dynamically, which causes a failure when refreshing dependencies. To avoid Kotlin metadata build issues, these have been pinned. - feat(codegen): use `kotlinPackage` as prefix for kotlin codegen - feat(gradle): `kotlinPackage` property in gradle plugin - test(codegen): add tests for custom kotlin package - test(gradle): add tests for generating with custom kotlin package Signed-off-by: Sam Gammon <[email protected]>
This change adds support for a new code-gen argument, `implementKSerializable`, which results in the annotation `kotlinx.serialization.Serializable` being added to `data` classes during codegen. Relates to discussion apple#185 - feat(codegen): add support for kotlin `Serializable` annotation - feat(gradle): add `implementKSerializable` argument - test(codegen): add test for KotlinX serialization support - test(codegen): add test for both Java and KotlinX serialization - test(gradle): add test for compiling with KotlinX serialization Signed-off-by: Sam Gammon <[email protected]>
- feat: support for build scans with gradle enterprise - feat: support for caching with buildless (inert without key) - feat: support for gradle java toolchains - feat: support for static analysis with detekt - feat: support for gradle toolchains - feat: support for toolchain vs. runtime target - feat: support for dynamic provisioning of toolchains - feat: support for static java checking with pmd - feat: enable typed project accessors, use them project-wide - feat: stricter repositories, locking for build classpath - feat: property to retarget java or kotlin bytecode versions - feat: parameter name integration between javac and kotlinc - feat: kotlin coverage support via `kover` plugin - feat: dependency verification for gradle build - feat: support for new gradle `jvm-test-suite` plugin - feat: reasonable local and remote build caching support - feat: project icon in intellij new ui - feat: aggregate reporting for tests, coverage, detekt - fix: repeatable/consistent archives from gradle - fix: don't list ephemeral spotless configurations in lockfiles - fix: make version catalog symbols available in `buildSrc` - fix: specify `rootProject.name` for `buildSrc` - fix: error when running `gradlew tasks` - fix: various java or gradle deprecations - fix: move all tool (linter, etc) versions into version catalog - chore: add testlogger for clearer test outcomes - chore: check build configuration with gradle doctor plugin - chore: generate initial suite of dependency verification material - chore: transition to property set syntax (`property = xyz`) - chore: cleanup uses of `buildDir` (becomes `layout.buildDirectory`) - chore: add Gradle Versions plugin for update checks - chore: upgrade Gradle → `8.6` (supports Java 21) - chore: upgrade Kotlin → `1.9.22` (build-time) - chore: upgrade KotlinX Serialization → `1.6.3` - chore: upgrade KotlinX HTML → `0.11.0` - chore: general dependency upgrades, where safe Not yet completed: - feat: signing of artifacts with sigstore - feat: embedding of SPDX SBOM in artifacts - feat: dependency vulnerability checks with owasp - test: checksum failures Signed-off-by: Sam Gammon <[email protected]>
- feat: github actions submission of dependency graph - feat: checks in gha: detekt, formatting, gradle wrapper - feat: check prs for vulnerable dependencies - feat: oss scorecards job - feat: run codeql on pr / push Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review, part 2. Includes the following:
- GraalVM 23.x support (latest), at Java 21
- Native JPMS support project-wide
- Lightweight plug-in system for
pkl-core
- PGO, strict image heap, and other native build enhancements
- Native CLI distribution packages
val devNativeImageFlags = listOfNotNull( | ||
"-Ob", | ||
if (!enablePgo) null else "--pgo-instrument", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Native Image now builds by default with -Ob
(optimizes build time). To build a "release" native binary, pass -PnativeRelease=true
.
When built in dev mode, the binary is instrumented with --pgo-instrument
when enablePgo
(a flag in this script) is flipped to true
.
val releaseCFlags: List<String> = listOf( | ||
"-O3", | ||
"-v", | ||
) | ||
|
||
val releaseNativeImageFlags = listOf( | ||
"-O3", | ||
"-march=native", | ||
if (!enablePgo) null else "--pgo=../profiles/$profileName", | ||
).plus(releaseCFlags.flatMap { | ||
listOf( | ||
"-H:NativeLinkerOption=$it", | ||
"--native-compiler-options=$it", | ||
) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In release mode, the Native Image now builds with:
-O3
-march=native
-H:NativeLinkerOption="-O3 -flto"
-H:CStandard=C11
-J-Dpolyglot.image-build-time.PreinitializeContextsWithNative=true
--native-compiler-options="-O3 -flto"
--pgo=...
--strict-image-heap
compileOnly(libs.kotlinStdlib) | ||
|
||
// JPMS module path | ||
modulepath(libs.kotlinStdlib) | ||
modulepath(libs.jlineReader) | ||
modulepath(libs.svmTruffle) | ||
modulepath(libs.truffleApi) | ||
modulepath(libs.truffleRuntime) | ||
modulepath(projects.pklCore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the modulepath
configuration in pkl-cli
to move dependencies to the --module-path
.
fun selectNativeHostExecutable(): TaskProvider<Exec> { | ||
return when { | ||
buildInfo.os.isMacOsX && (buildInfo.arch == "amd64") -> macExecutableAmd64 | ||
buildInfo.os.isMacOsX && (buildInfo.arch == "aarch64") -> macExecutableAarch64 | ||
buildInfo.os.isLinux && (buildInfo.arch == "amd64") -> linuxExecutableAmd64 | ||
buildInfo.os.isLinux && (buildInfo.arch == "aarch64") -> linuxExecutableAarch64 | ||
buildInfo.os.isWindows -> windowsAmd64 | ||
else -> error("No host binary could be selected; please check your OS for Pkl support") | ||
} | ||
} | ||
|
||
val nativeHostExecutable by tasks.registering { | ||
selectNativeHostExecutable().get().let { | ||
dependsOn(it) | ||
inputs.files(it.outputs.files) | ||
outputs.files(it.outputs.files) | ||
outputs.cacheIf { true } | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkl-cli
now provides a nativeHostExecutable
task, which will use the best available native CLI executable target. This is mounted as an artifact so that smoke testing can be added for native CLIs.
Build avoidance was preserved here; running ./gradlew build
should not trigger native builds. These targets must specifically be requested with ./gradlew :pkl-cli:nativeHostExecutable
or a directly named native binary target (e.g. macExecutableAmd64
).
fun createArchiveTasks( | ||
exec: Provider<Exec>, | ||
outputFile: File, | ||
release: Boolean = enableRelease, | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pkl-cli
module now creates Gradle distributions for the native CLI, in both zip and tar. These are the distributions I am posting to the PR for download.
/** Describes the public SPI for Pkl engine plugins */ | ||
public interface PklPlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New lightweight plugin interface provided by pkl-core
over an SPI; the hook points provided to plug-ins only include one so far, onContextCreated
, which has an opportunity to modify the org.graalvm...Context.Builder
before it is built into a Context
instance and used by Pkl.
/** Pkl core engine version */ | ||
public static final String PKL_CORE_VERSION = "0.26.0-dev"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certain constants moved here so they can be exposed to other code (for example, telling plugins the current version of pkl-core
).
@Override | ||
protected boolean patchContext(VmContext context, Env newEnv) { | ||
// no-op | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is now required to use shared/pre-initialized contexts in GraalVM. It is meant to inform a hot context of a new Env
it should use. I've implemented it as a no-op because I don't see any use of Env
anyway.
var context = Context.newBuilder("pkl").engine(PKL_ENGINE).build(); | ||
var context = PKL_ENGINE.get().build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VmUtils
now consults the VmEngineManager
to obtain the engine; in doing so, the onContextCreated
plugin hook event is dispatched.
@PklExperimental | ||
Property<Boolean> getImplementKSerializable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example use of @PklExperimental
; marking this symbol this way will throw an opt-in warning for downstream Gradle Kotlin DSL users if they try to use implementKSerializable
.
@holzensp I have re-requested review with a new batch of changes, with another self-review listed above. This gets into bigger refactors like Even though these are bigger changes, with the benefit of hindsight on this fork, they should be easy to chop up into reviewable portions. Thanks for the reviews and for merging my PRs so far 😄 cc / @stackoverflow Edit: Reports and other material
|
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Terribly sorry; I was entirely off-the-grid for a couple of weeks. Circling back to this tomorrow. |
@holzensp No problem! Take your time and hope your trip off-grid was good 😁. There's a lot here, this fork is alive and continues to evolve. The latest summary of features supported ahead of
The plug-in system allows me to customize the Please consider me available for anything needed for Pkl, and I'm on the Discord too. @bioball has my cell and knows I'm committed to help. |
@sgammon A note on the There are minor differences, but I think they are not related to the main reason why previously it was useful to replace |
Summary
Not for merge
This is a massive PR that upgrades the Gradle build in a big way. I did this sort of for fun, over the weekend, while working on other PRs which are reasonable and not enormous.
We are intending to use and extend Pkl so we could maintain a fork, and I have no expectation that this will get merged as-is. That being said, since we've done the leg work on some of this stuff and it could be useful, I wanted to file and offer any of the below as a menu of changes the Pkl team could get upstreamed from us if desired.
There are a handful of test failures and checksums have changed, which I am fixing. Total broken tests number less than 10, I believe, and the breakages seem fixable.
Feedback is of course welcome but since this PR isn't designed to be merged, such feedback would be incorporated in another PR, which is smaller and reviewable.
Thank you to the Pkl team for a very cool project!
Update: Feb 21st
New batch of changes pushed with the following:
buildSrc
refactored toincludeBuild("build-logic")
(here's why)23.1.2
)--strict-image-heap
pkl
truffle modulemodule-info
)-Ob
pkl-core
enginefeat: usenote: rolling this back bcepsilon
gc in native binarypkl server
Initial native binary benchmarks show a modest improvement:
Download URLs to try it out
aarch64
: zip, tar.gzUpdate: Split-out PRs (Feb 20th)
Changes enclosed
0) Fun developer ergonomics stuff.
1) Build upgrades.
jvm-test-suite
pluginsproperty = value
syntax instead ofproperty.set(value)
1.9.22
(for build)8.6
21
2) Supply chain security.
3) GitHub project integration/ergonomics
4) Library updates / upgrades
1.9.22
(for codegen and scripting)Full changelog
Expand to see full changelog
Not yet completed