Fix: Native Compile #649
Merged
Fix: Native Compile #649
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Sam Gammon <sam@elide.ventures>
Signed-off-by: Sam Gammon <sam@elide.ventures>
sgammon
added
bug
WalkthroughWalkthroughThe project underwent significant refactoring, focusing on simplifying its configuration and dependencies. Key changes include the removal of WebAssembly and specific tests, a shift from enterprise to community editions, and a disabling of various features and plugins. These modifications aim to streamline the build process, reduce complexity, and enhance compatibility across different environments. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to path filters (6)
docs/reports/images/d.gifis excluded by:!**/*.gifdocs/reports/images/d.pngis excluded by:!**/*.pngdocs/reports/images/throbber.gifis excluded by:!**/*.gifgradle/elide.versions.tomlis excluded by:!**/*.tomlproto/buf.pb.binis excluded by:!**/*.binproto/buf.pb.json.gzis excluded by:!**/*.gz
Files selected for processing (15)
- .github/workflows/args.test.txt (1 hunks)
- .github/workflows/job.test.yml (2 hunks)
- .gitignore (1 hunks)
- gradle.properties (1 hunks)
- packages/cli/build.gradle.kts (17 hunks)
- packages/cli/src/main/kotlin/elide/tool/cli/cmd/repl/ToolShellCommand.kt (1 hunks)
- packages/embedded/build.gradle.kts (7 hunks)
- packages/graalvm/api/graalvm.api (1 hunks)
- packages/graalvm/build.gradle.kts (1 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/core/internals/graalvm/GraalVMEngine.kt (3 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/js/JsRuntime.kt (2 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/plugins/js/JavaScript.kt (4 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/plugins/js/JavaScriptConfig.kt (1 hunks)
- packages/graalvm/src/test/kotlin/elide/runtime/core/ContextTest.kt (3 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/dependencies/DependencyResolutionConventions.kt (4 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/args.test.txt
- .gitignore
Additional comments: 30
gradle.properties (1)
- 40-40: The update from
ENTERPRISEtoCOMMUNITYforelide.graalvm.variantaligns with the PR's objective to transition to GraalVM Community Edition. Ensure that all other configurations and dependencies in the project are compatible with the Community Edition.packages/graalvm/src/main/kotlin/elide/runtime/plugins/js/JavaScriptConfig.kt (1)
- 47-48: Swapping the order of
fsandutilin themoduleReplacementsmap appears to be a minor change for organizational purposes and should not affect functionality. It's always good to document the reasoning behind such changes for future reference.tools/elide-build/src/main/kotlin/elide/internal/conventions/dependencies/DependencyResolutionConventions.kt (3)
- 22-33: The addition of logic to determine if enterprise features are enabled and the introduction of a list of Truffle Enterprise artifacts to omit when building against GraalVM CE align with the PR's objectives. Ensure that these exclusions are correctly applied in the build process.
- 125-125: Updating the handling of dependency pins for
org.bouncycastleto a specific version is a good practice for ensuring consistency and compatibility. Verify that this version aligns with the project's security and functionality requirements.- 155-157: The refactoring for the exclusion of certain modules based on GraalVM and enterprise features is crucial for the transition to GraalVM CE. Ensure that all necessary exclusions are correctly applied to avoid including incompatible or unnecessary dependencies.
packages/graalvm/src/test/kotlin/elide/runtime/core/ContextTest.kt (3)
- 21-21: Adding an import for
kotlin.test.Ignoreis necessary for the modifications made to the test annotations. Ensure that the usage of@Ignoreis well-documented, including reasons and conditions for future re-enabling.- 59-74: The addition of
testIntrinsicBindingsJsto test JavaScript bindings is a good practice for ensuring feature robustness. Ensure that this test covers all relevant scenarios for JavaScript bindings.- 76-85: The addition of
testIntrinsicBindingsPyto test Python bindings, although marked with@Ignore, indicates an area for future development or investigation. Document the reasons for ignoring this test and any plans for addressing the underlying issues.packages/graalvm/src/main/kotlin/elide/runtime/plugins/js/JavaScript.kt (3)
- 16-16: The addition of an import statement for
org.graalvm.polyglot.Engineis necessary for the changes made to JavaScript support, specifically for determining WASM support. Ensure that this import is used appropriately throughout the file.- 67-115: The reordering and modification of options related to JavaScript features, character set, CommonJS, locale, and more are aimed at enhancing JavaScript support within the project. Verify that these adjustments do not negatively impact JavaScript functionality and compatibility.
- 144-146: Refactoring the
wasmSupportedproperty initialization to use a lazy delegate ensures that the WASM support check is performed only once and only when needed. This is a good practice for improving performance and maintainability.packages/graalvm/build.gradle.kts (1)
- 260-261: The addition of
libs.graalvm.polyglotandlibs.graalvm.polyglot.jsdependencies without exclusions is noted. Ensure that these dependencies are indeed required for the project's objectives related to GraalVM CE compatibility and native compilation optimization. If these libraries are not directly used or if there are concerns about their impact on the build size or performance, consider evaluating their necessity further.packages/graalvm/src/main/kotlin/elide/runtime/core/internals/graalvm/GraalVMEngine.kt (2)
- 158-165: The introduction of
ENABLE_ENTERPRISE,ENABLE_ISOLATES, andENABLE_AUX_CACHEconstants with their respective settings aligns with the PR's objective to transition to GraalVM Community Edition and optimize for native compilation. However, settingENABLE_ENTERPRISEtofalseand adjusting the other flags accordingly is crucial to ensure compatibility with GraalVM CE. It's good practice to document the rationale behind these settings, especially forENABLE_AUX_CACHEbeing set tofalse, to clarify the decision-making process for future maintainers.- 249-256: The conditional check for
ENABLE_ENTERPRISEbefore enabling theengine.CachePreinitializeContextoption is a good practice, ensuring that enterprise-specific features are not inadvertently enabled in the Community Edition. This careful consideration helps maintain compatibility with GraalVM CE. Additionally, the dynamic generation of the cache path using the process ID is a thoughtful approach to avoid conflicts and ensure that each instance has its unique cache, enhancing the build process's robustness.packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/js/JsRuntime.kt (1)
- 158-161: The removal of
js.operator-overloadingandjs.printproperties suggests a refinement of the JavaScript runtime's feature set, potentially to streamline the runtime or address compatibility issues. While these changes may improve the runtime's behavior or performance, it's crucial to ensure that they do not inadvertently remove functionality that existing scripts or applications rely on. Consider verifying the impact of these removals on the project's JavaScript codebase and any external dependencies that may expect these features..github/workflows/job.test.yml (3)
- 276-276: The
detekttask has been adjusted with additional flags and properties. It's important to ensure that these changes align with the project's goals for code quality and static analysis. Specifically, the-x apiCheckflag excludes theapiChecktask from execution, which might impact the enforcement of API compatibility checks. Consider whether this exclusion aligns with the project's quality assurance processes.- 276-276: The
--build-cacheflag is used, which enables the Gradle build cache. This can significantly improve build times by reusing outputs from previous executions. However, it's crucial to ensure that the build cache is properly configured to avoid incorrect build results due to stale or inappropriate caching. Review the cache configuration to ensure it's correctly set up for the project's needs.- 276-276: The
-PbuildSamples=false,-PbuildDocs=false, and-PbuildDocsSite=falseproperties are used to disable building samples, documentation, and the documentation site, respectively. This is likely an optimization for CI builds to focus on essential tasks. Ensure that this decision aligns with the project's CI strategy and that documentation and samples are built and verified in other parts of the CI pipeline or in a separate workflow to maintain their quality and up-to-dateness.packages/embedded/build.gradle.kts (6)
- 120-120: The commented-out argument
// "ALL-UNNAMED",injvmCompileArgsmight have been disabled for a specific reason, possibly related to compatibility with GraalVM CE. However, it's important to ensure that this change does not inadvertently affect the module's ability to access internal APIs of other modules, especially ifALL-UNNAMEDwas initially included to enable such access.- 213-213: Adding
-H:DeadlockWatchdogInterval=15tocommonNativeArgsis a good practice for detecting deadlocks during the native image build. However, ensure that this interval is appropriate for your build environment and does not lead to false positives, especially in slower build environments where certain operations might take longer.- 635-635: Adjusting the memory settings in
darwinOnlyArgswith-J-Xmx32gis a significant change that aims to allocate more memory for the build process on Darwin systems. This adjustment should be carefully considered based on the available system resources and the actual memory requirements of the build process. Over-allocating memory could lead to inefficient use of resources, especially in shared or constrained environments.- 446-451: The addition of JANSI related classes in
initializeAtBuildTimeis a good practice for ensuring that these classes are initialized during the build time of the native image, which can improve the runtime performance and compatibility of applications using ANSI console features. However, verify that all necessary JANSI classes are included and that there are no additional classes required for your specific use case.- 910-942: The conditional inclusion of GraalVM components based on
oracleGvmandenableExperimentalflags is a good approach to manage dependencies dynamically based on the build configuration. However, ensure that these conditions accurately reflect the project's requirements for GraalVM components and that there are no missing or unnecessary modules being included. This is particularly important for maintaining the build's performance and avoiding the inclusion of unused components.- 969-969: Adding the Bouncy Castle library for cryptography with
implementation(libs.bouncycastle)and related runtime dependencies is a significant change that enhances the project's cryptographic capabilities. Ensure that the Bouncy Castle version is compatible with the project's requirements and that its inclusion does not introduce any licensing or security issues. Additionally, verify that all necessary Bouncy Castle modules are included for the cryptographic features you intend to use.packages/cli/build.gradle.kts (6)
- 97-97: Setting
oracleGvmtofalsealigns with the transition to GraalVM Community Edition. However, ensure that all related configurations and conditional checks throughout the script are updated accordingly to reflect this change.- 100-110: Disabling various features (
enablePython,enableRuby, etc.) is consistent with the objective to optimize the build for native targets. It's important to verify that these features are indeed not required for the CLI's functionality or if alternative solutions are in place for any functionality they provided.Verification successful
The verification process has revealed that the
enablePythonandenableRubyflags are indeed used within thepackages/clibuild script to conditionally include dependencies and configure the build process based on their enabled state. The flags are also present in thepackages/embeddedbuild script with a different configuration (set totrue), indicating a deliberate choice to enable these features in one part of the project while disabling them in another. This suggests that the decision to disable these features in the CLI package is intentional and likely serves the project's objectives to optimize or tailor the build for specific requirements. Given this context, the review comment is consistent with the findings, and it's important to ensure that the CLI's functionality is not adversely affected by these configurations.* 281-284: Adding missing types with `implementation(libs.jetty.npn)` and `implementation(libs.jetty.alpn)` is a good practice to ensure compatibility and functionality. Ensure these libraries are compatible with the versions of other dependencies and the runtime environment. * 628-639: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [631-673]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if any code paths rely on these disabled features. grep -R "enablePython" . || echo "No references to enablePython found." grep -R "enableRuby" . || echo "No references to enableRuby found."Length of output: 1298
The configuration for common native image generation arguments (
commonNativeArgs,commonGvmArgs,nativeImageBuildDebug) appears well-structured and aligns with the objectives for optimizing native compilation. However, it's crucial to ensure that all flags and options used are supported and recommended for the target GraalVM Community Edition version.
- 1065-1069: The use of system properties to control the preinitialization of the VM engine based on the
oracleGvmflag and CI environment is a smart approach to manage build behavior dynamically. However, ensure that the logic correctly reflects the intended behavior in both CI and non-CI environments, especially sinceoracleGvmis set tofalse.- 628-639: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [631-760]
The extensive configuration for native image generation, including
commonGvmArgs,commonNativeArgs,dashboardFlags,debugFlags, andexperimentalFlags, demonstrates a thorough approach to optimizing the build for native compilation. It's important to review these configurations for compatibility with GraalVM Community Edition and the specific requirements of the project.
packages/cli/src/main/kotlin/elide/tool/cli/cmd/repl/ToolShellCommand.kt
Show resolved
Hide resolved
packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/js/JsRuntime.kt
Show resolved
Hide resolved
packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/js/JsRuntime.kt
Show resolved
Hide resolved
| @@ -242,7 +229,7 @@ kapt { | |||
| kotlin { | |||
| target.compilations.all { | |||
| kotlinOptions { | |||
| allWarningsAsErrors = true | |||
| allWarningsAsErrors = false | |||
There was a problem hiding this comment.
Setting allWarningsAsErrors to false may help avoid build failures due to warnings, especially during the transition to a new Kotlin version or other major changes. However, it's generally a good practice to address warnings rather than suppress them, as they can indicate potential issues or areas for improvement.
Consider addressing the underlying causes of warnings where feasible, to maintain code quality and stability.
There was a problem hiding this comment.
@sgammon: If there's anything specific you need help with, please let me know.
|
@sgammon heads up looks like files under |
sgammon
commented
Feb 17, 2024
Comment on lines
+1549
to
+1558
| // PYTHON -> install(elide.runtime.plugins.python.Python) { | ||
| // logging.debug("Configuring Python VM") | ||
| // installIntrinsics(intrinsics, GraalVMGuest.PYTHON, versionProp) | ||
| // } | ||
|
|
||
| // RUBY -> install(elide.runtime.plugins.ruby.Ruby) { | ||
| // logging.debug("Configuring Ruby VM") | ||
| // installIntrinsics(intrinsics, GraalVMGuest.RUBY, versionProp) | ||
| // } |
There was a problem hiding this comment.
will refactor this in next PR
Comment on lines
119
to
+120
| "--enable-native-access=" + listOfNotNull( | ||
| "ALL-UNNAMED", | ||
| // "ALL-UNNAMED", |
darvld
requested changes
Feb 17, 2024
darvld
approved these changes
Feb 17, 2024
- fix: native compilation of `cli` entrypoint - fix: errant logging for main entrypoints - fix: native optimized compilation of `embedded` module - fix: ability to build against GraalVM CE - fix: only apply wasm flags if language is supported - fix: double `detekt` call in ci jobs - fix: various warning cleanups Signed-off-by: Sam Gammon <sam@elide.ventures>
sgammon
force-pushed
the
fix/native-compile
branch
from
9bd972b to
0ffbec6
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (6)
docs/reports/images/d.gifis excluded by:!**/*.gifdocs/reports/images/d.pngis excluded by:!**/*.pngdocs/reports/images/throbber.gifis excluded by:!**/*.gifgradle/elide.versions.tomlis excluded by:!**/*.tomlproto/buf.pb.binis excluded by:!**/*.binproto/buf.pb.json.gzis excluded by:!**/*.gz
Files selected for processing (16)
- .github/workflows/job.test.yml (2 hunks)
- .gitignore (1 hunks)
- gradle.properties (1 hunks)
- packages/cli/build.gradle.kts (17 hunks)
- packages/cli/src/main/kotlin/elide/tool/cli/cmd/repl/ToolShellCommand.kt (2 hunks)
- packages/cli/src/main/kotlin/elide/tool/control/AbstractScriptEngineFactory.kt (1 hunks)
- packages/embedded/build.gradle.kts (7 hunks)
- packages/embedded/src/main/kotlin/elide/embedded/ElideEmbedded.kt (1 hunks)
- packages/graalvm/api/graalvm.api (1 hunks)
- packages/graalvm/build.gradle.kts (1 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/core/internals/graalvm/GraalVMEngine.kt (3 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/js/JsRuntime.kt (2 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/plugins/js/JavaScript.kt (4 hunks)
- packages/graalvm/src/main/kotlin/elide/runtime/plugins/js/JavaScriptConfig.kt (1 hunks)
- packages/graalvm/src/test/kotlin/elide/runtime/core/ContextTest.kt (3 hunks)
- tools/elide-build/src/main/kotlin/elide/internal/conventions/dependencies/DependencyResolutionConventions.kt (4 hunks)
Files skipped from review as they are similar to previous changes (13)
- .github/workflows/job.test.yml
- .gitignore
- gradle.properties
- packages/cli/src/main/kotlin/elide/tool/cli/cmd/repl/ToolShellCommand.kt
- packages/embedded/build.gradle.kts
- packages/graalvm/api/graalvm.api
- packages/graalvm/build.gradle.kts
- packages/graalvm/src/main/kotlin/elide/runtime/core/internals/graalvm/GraalVMEngine.kt
- packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/js/JsRuntime.kt
- packages/graalvm/src/main/kotlin/elide/runtime/plugins/js/JavaScript.kt
- packages/graalvm/src/main/kotlin/elide/runtime/plugins/js/JavaScriptConfig.kt
- packages/graalvm/src/test/kotlin/elide/runtime/core/ContextTest.kt
- tools/elide-build/src/main/kotlin/elide/internal/conventions/dependencies/DependencyResolutionConventions.kt
Additional comments: 12
packages/embedded/src/main/kotlin/elide/embedded/ElideEmbedded.kt (1)
- 66-66: The
@Suppress("UNUSED_PARAMETER")annotation has been added to theElideEmbeddedclass. It's important to verify the necessity of suppressing unused parameter warnings at the class level. If this is intended for specific methods, consider applying it directly to those methods to maintain clarity and avoid suppressing potentially useful warnings elsewhere in the class. Is there a specific reason for its placement at the class level that might not be immediately apparent?packages/cli/src/main/kotlin/elide/tool/control/AbstractScriptEngineFactory.kt (3)
- 455-456: Renaming the parameter from
valuetonewValuein thesetValuemethod improves clarity, especially in the context of setting a new value where an old value might exist. This change enhances readability and makes the method's purpose more explicit.- 464-466: Changing the parameter name from
nametokeyin theputmethod aligns with common terminology for maps or key-value stores, making the method signature more intuitive and consistent with standard practices.- 470-471: The parameter renaming from
toMergetofromin theputAllmethod clarifies the source of the data being merged into the current bindings. This change enhances the method's readability and aligns with common naming conventions for similar operations.packages/cli/build.gradle.kts (8)
- 97-97: Setting
oracleGvmtofalsealigns with the transition to GraalVM Community Edition. This change is crucial for ensuring that the build configurations are compatible with the CE variant of GraalVM.- 100-110: Disabling features such as Python, Ruby, and other non-essential plugins is a good optimization for native compilation. This approach helps in reducing the build time and the size of the final binary, which is beneficial for native targets.
- 149-149: The conditional application of the
com.jakewharton.mosaicplugin based on theenableMosaicflag is a good practice. It ensures that the plugin is only applied when necessary, contributing to the optimization of the build process.- 281-284: Adding missing types such as
libs.jetty.npnandlibs.jetty.alpnis important for ensuring compatibility and functionality, especially when dealing with HTTP/2 or other network protocols that may require ALPN support.- 345-345: Including
libs.jline.terminal.jnaamong the dependencies is essential for enhancing terminal capabilities, especially for native images that may interact with the console or terminal directly.- 438-438: The addition of
libs.graalvm.truffle.apito the API dependencies is crucial for projects leveraging GraalVM's Truffle API for implementing languages or embedding GraalVM languages. This ensures that the necessary Truffle API components are available during compilation.- 628-639: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [631-673]
The configuration of common GraalVM native image arguments, including the handling of experimental features and debug settings, is well-structured. However, it's important to ensure that these settings are aligned with the project's requirements for native compilation and debugging.
Verification successful
The verification process has confirmed that the project intentionally utilizes experimental features and debug settings, with mechanisms in place to toggle these based on specific needs or environments. The use of
enableExperimentalandnativeImageBuildDebugacross various parts of the project is aligned with the project's requirements for native compilation and debugging. The configuration and conditional inclusion of these settings suggest a structured and cautious approach to their application.* 1065-1069: The use of platform-specific arguments for Windows, including the preinitialization setting based on `oracleGvm`, is a good practice for optimizing native image builds. However, since `oracleGvm` is set to `false`, ensure that any Oracle-specific configurations are not required for the project.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the experimental features enabled are necessary and supported by GraalVM CE. grep -R "enableExperimental" . --include=*.kts # Ensure debug settings are only enabled for appropriate build profiles. grep -R "nativeImageBuildDebug" . --include=*.ktsLength of output: 2897
| @@ -242,7 +229,7 @@ kapt { | |||
| kotlin { | |||
| target.compilations.all { | |||
| kotlinOptions { | |||
| allWarningsAsErrors = true | |||
| allWarningsAsErrors = false | |||
There was a problem hiding this comment.
Setting allWarningsAsErrors to false can be helpful in avoiding build failures due to warnings. However, it's important to address the underlying causes of these warnings to maintain code quality. Consider enabling this setting once the warnings have been resolved.
- allWarningsAsErrors = false
+ allWarningsAsErrors = true // After resolving warningsCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| allWarningsAsErrors = false | |
| allWarningsAsErrors = true // After resolving warnings |
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #649 +/- ##
==========================================
- Coverage 36.91% 36.79% -0.12%
==========================================
Files 469 469
Lines 13906 13882 -24
Branches 1956 1951 -5
==========================================
- Hits 5133 5108 -25
- Misses 8424 8426 +2
+ Partials 349 348 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes for native builds in the
cliandembeddedmodules.Summary by CodeRabbit
.gitignoreto include.codebase/anddocs/reports/.