Replace the agent-filter file #78
Merged
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
melix
force-pushed
the
cc/replace-filter
branch
3 times, most recently
from
6e51eda
to
f8bb142
Compare
The previous call was displaying a warning but using error level.
Instead, the task should use the `ExecOperations`. It basically uses delegation instead of inheritance. This has several advantages: - it makes sure that we can explicitly list all inputs to the task - it reduces the API surface of the task itself - it will make sure the task is compatible with the Gradle configuration cache The task is not configuration cache ready yet, since it uses the project in the task action: this will be fixed later. Signed-off-by: Cedric Champeau <[email protected]>
This removes the deprecated native image task. Bridging "old" tasks like that is in any case not right because if some tasks in the build were using the old image outputs, then they wouldn't be wired properly anymore.
This is in preparation for cleanup of the extension configuration.
This commit refactors the native image configuration extension to be more Gradle idiomatic. In particular: - creation of the properties is now delegated to Gradle, instead of directly instantiating - setter methods were removed, as Gradle is responsible for providing a consistent DSL from property definitions. In particular, it is responsible for creating setter-like methods in the Groovy DSL (Kotlin DSL will come at some point) - setter methods which were actually appending were confusing, and removed - only some "appending" methods were kept, for the DSL to be a bit nicer, although they should probably be removed at some point too, once the DSL generated by Gradle gets nicer The wiring of the internal state hasn't been changed yet.
Previously to this commit, there were 2 extensions of 2 different types to configure the native extension, depending on whether it was for tests or not. It wasn't necessary, as the configuration of the native image task is the same in both cases, it's simply the values of the extensions which differ slightly. Therefore, this commit simplifies everything by using a _single_ extension type. Similarly, there were 2 "build image" task types, when in practice the building mechanism is the same in both cases. Therefore, the 2 tasks were replaced with a single one, `BuildNativeImageTask`, which is setup appropriately in both cases. Last, for the same reason, the 2 "run" task types have been replaced with a single version. Last but not least, configuration of the tasks is now properly done by the plugin, instead of being mixed between the plugin configuration and the task creation. There were ordering issues due by the fact that the configuration of the task was mutating its own state. Instead, tasks are now properly isolated, without any knowledge of their configuration, and the plugin is responsible for injecting the configuration of the tasks. This makes it possible, for example, to derive the name of the test image from the name of the main image. It's worth noting that this commit didn't fully refactor the configuration phase, because there's still fishy configuration of the 'agent' side of things. This will be done in a subsequent commit.
The test discovery file was generated, but never used in the native image, which caused the use of the discovery mode instead.
This commit reworks how the agent is injected on the `run` and `test` tasks. There is now a task which role is to copy the filter json file, but more importantly the value of the `agent` property is not read eagerly. A new test has been added to make sure that the agent is properly injected and that the configuration files are generated. This doesn't work properly for GraalVM versions <21.1. The "persistConfig" property is temporarily _unused_, we have to decide what to do with it.
A new test suite, `configCacheFunctionalTest` enables the configuration cache. But currently the test suite fails, not because of the compatibility itself, but because for some reason Gradle fails to write its report.
Because of a weird ordering issue, it was possible that the native test image task configuration was executed _before_ the test task configuration, which caused an error when tests were executed.
There were several issues with this flag: 1. it persists outputs in the (re)sources directories, which means that potentially, during a build, a task can change the contents of the (re)sources. Therefore, there's a potential ordering and non reproducibility issue based on when a task sees the resources. 2. it arbitrarily chooses a source set to persist data, but in practice this shouldn't live in a source set, since the result of the analysis, and actually the resources needed, may be split accross different source sets (think Java vs Kotlin sources) Therefore we agreed on removing it. A user can still access the result of the agent analysis via the build directory, and copy them manually if needed, or they can implement their own task, with their own risks taking, if they really want to do this.
This would allow use of any GraalVM version (Community or Enterprise).
In case the tests were executed directly, the working directory was set to the project directory, leading to test results written in the root dir instead of the build directory.
- remove unused persistConfig constant - don't use the `.cmd` extension under Windows for `-H:Name` - fix detection of GraalVM EE in test fixtures - fix formatting
This commit backports the fix from graalvm#80
This commit fixes a problem with the `run` task which could be accidentally executed if `nativeBuild` was called *and* that the `run` task was eagerly configured, even if the agent property wasn't set.
This commit replaces the adhoc `agent-filter.json` file with a post-processing step. Basically, instead of passing an access filter file to the agent, we now post-process the generated JSON files to remove entries that we don't need. There are several advantages doing this: 1. user can rewire the image build task to use different files 2. filering can be made more configurable in the future 3. the plugin is now compatible with older GraalVM releases The current way the files are filtered, though, is very rough. Because there's no official JSON schemas for the generated files, it's an adhoc, deep inspection algorithm which filters known patterns.
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.
This PR replaces the agent-filter.json file with a task which processes the output of the agent instead.
It builds on top of #71 so don't be scared by the number of commits, look at c690233 ;)