Skip to content

Commit ab8551b

Browse files
marchermansmarchermans
authored
Implement OOM detection in decompiler execution and update related configurations (#284)
1 parent 6e276ab commit ab8551b

17 files changed

Lines changed: 254 additions & 64 deletions

File tree

‎.github/workflows/test-prs.yml‎

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
name: Test PRs
2-
run-name: Tests for PR ${{ github.event.pull_request.number }}
2+
run-name: Tests for ${{ github.event.pull_request.number || github.event.inputs.gradle-version }}
33

44
on:
55
pull_request:
@@ -8,17 +8,40 @@ on:
88
- opened
99
- ready_for_review
1010
- reopened
11+
workflow_dispatch:
12+
inputs:
13+
gradle-version:
14+
description: 'Gradle version to use (required for manual run)'
15+
required: true
16+
default: ''
1117

1218
concurrency:
1319
group: ci-${{ github.head_ref || github.run_id }}
1420
cancel-in-progress: true
1521

1622
jobs:
23+
set-gradle-version:
24+
name: Set Gradle Version
25+
runs-on: ubuntu-latest
26+
outputs:
27+
gradle-version: ${{ steps.set-version.outputs.gradle-version }}
28+
steps:
29+
- name: Determine gradle version
30+
id: set-version
31+
run: |
32+
if [ "${{ github.event_name }}" = "pull_request" ]; then
33+
echo "gradle-version=default" >> "$GITHUB_OUTPUT"
34+
else
35+
echo "gradle-version=${{ github.event.inputs.gradle-version }}" >> "$GITHUB_OUTPUT"
36+
fi
37+
1738
setup:
1839
name: Setup
1940
runs-on: ubuntu-latest
41+
needs: set-gradle-version
2042
outputs:
2143
tests-to-run: ${{ steps.test.outputs.tests-to-run }}
44+
gradle-version: ${{ needs.set-gradle-version.outputs.gradle-version }}
2245
steps:
2346
- name: Checkout repository
2447
uses: neoforged/actions/checkout@main
@@ -42,10 +65,12 @@ jobs:
4265
#!/bin/bash
4366
./.github/scripts/collect-tests.sh
4467
45-
4668
build:
4769
name: Build
4870
runs-on: ubuntu-latest
71+
needs: set-gradle-version
72+
env:
73+
GRADLE_VERSION: ${{ needs.set-gradle-version.outputs.gradle-version }}
4974
steps:
5075
- name: Checkout repository
5176
uses: neoforged/actions/checkout@main
@@ -60,18 +85,20 @@ jobs:
6085
uses: gradle/gradle-build-action@v3
6186

6287
- name: Build
63-
run: ./gradlew --info -s -x assemble
88+
run: ./gradlew --info -s -x assemble -Ptraining-wheels.gradle-version=${{ env.GRADLE_VERSION }}
6489

6590
test:
6691
name: "${{ matrix.test.displayName }} (${{ matrix.os }})"
6792
runs-on: "${{ matrix.os }}-latest"
68-
needs: setup
93+
needs: [setup, set-gradle-version]
6994
strategy:
7095
max-parallel: 15
7196
fail-fast: false
7297
matrix:
7398
test: ${{ fromJSON(needs.setup.outputs.tests-to-run) }}
7499
os: [ubuntu, windows, macos]
100+
env:
101+
GRADLE_VERSION: ${{ needs.set-gradle-version.outputs.gradle-version }}
75102
steps:
76103
- name: Checkout repository
77104
uses: neoforged/actions/checkout@main
@@ -87,11 +114,11 @@ jobs:
87114

88115
- name: Test
89116
if: ${{ matrix.test.filter != null }}
90-
run: ./gradlew --info -s ${{ matrix.test.path }} --tests "${{ matrix.test.filter }}"
117+
run: ./gradlew --info -s ${{ matrix.test.path }} --tests "${{ matrix.test.filter }}" "-Ptraining-wheels.gradle-version=${{ env.GRADLE_VERSION }}"
91118

92119
- name: Test
93120
if: ${{ matrix.test.filter == null }}
94-
run: ./gradlew --info -s ${{ matrix.test.path }}
121+
run: ./gradlew --info -s ${{ matrix.test.path }} "-Ptraining-wheels.gradle-version=${{ env.GRADLE_VERSION }}"
95122

96123
# Always upload test results
97124
- name: Merge Test Reports
@@ -123,7 +150,7 @@ jobs:
123150
run: |
124151
# We need to remove all invalid characters from the test name:
125152
# Invalid characters include: Double quote ", Colon :, Less than <, Greater than >, Vertical bar |, Asterisk *, Question mark ?, Carriage return \r, Line feed \n, Backslash \, Forward slash /
126-
NAME=$(echo "${{ matrix.test.displayName }}" | tr '":<>|*?\\/' '-' | tr -d ' ')
153+
NAME=$(echo "${{ matrix.test.displayName }}" | tr '":<>|*?\\/' '-' | tr -d ' ')
127154
# Check if the GITHUB_OUTPUT is set
128155
echo "Determined name to be $NAME-${{ matrix.os }}"
129156
if [ -z "$GITHUB_OUTPUT" ]; then
@@ -191,4 +218,3 @@ jobs:
191218
with:
192219
script: |
193220
core.setFailed('Test build failure!')
194-

‎README.md‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -486,17 +486,17 @@ subsystems {
486486

487487
## Advanced Settings
488488

489-
### Override Decompiler Settings
489+
### <a id="common-decompiler-settings" /> Override Decompiler Settings
490490

491491
The settings used by the decompiler when preparing Minecraft dependencies can be overridden
492492
using [Gradle properties](https://docs.gradle.org/current/userguide/project_properties.html).
493493
This can be useful to run NeoGradle on lower-end machines, at the cost of slower build times.
494494

495-
| Property | Description |
496-
|----------------------------------------------|----------------------------------------------------------------------------------------------------------------------------|
497-
| `neogradle.subsystems.decompiler.maxMemory` | How much heap memory is given to the decompiler. Can be specified either in gigabyte (`4g`) or megabyte (`4096m`). |
498-
| `neogradle.subsystems.decompiler.maxThreads` | By default the decompiler uses all available CPU cores. This setting can be used to limit it to a given number of threads. |
499-
| `neogradle.subsystems.decompiler.logLevel` | Can be used to override the [decompiler loglevel](https://vineflower.org/usage/#cmdoption-log). |
495+
| Property | Description |
496+
|----------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
497+
| `neogradle.subsystems.decompiler.maxMemory` | How much heap memory is given to the decompiler. Can be specified either in gigabyte (`4g`) or megabyte (`4096m`). |
498+
| `neogradle.subsystems.decompiler.maxThreads` | By default the decompiler uses all available CPU cores. This setting can be used to limit it to a given number of threads, the lower the amount of threads the less memory is consumed. If set to 0 then the limit is disabled. |
499+
| `neogradle.subsystems.decompiler.logLevel` | Can be used to override the [decompiler loglevel](https://vineflower.org/usage/#cmdoption-log). |
500500

501501
### Override Recompiler Settings
502502

‎build.gradle‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,12 @@ subprojects.forEach { subProject ->
228228
classpath = evalSubProject.sourceSets.functionalTest.runtimeClasspath
229229

230230
it.extensions.add('test-source-set', evalSubProject.sourceSets.functionalTest)
231+
232+
if (evalSubProject.rootProject.getProperties().get("training-wheels.gradle-version") != null)
233+
it.systemProperty(
234+
"training-wheels.gradle-version",
235+
evalSubProject.rootProject.getProperties().get("training-wheels.gradle-version")
236+
)
231237
}
232238

233239
//Wire them up so they run as part of the check task (and as such through build, but not through test!)

‎common/src/main/java/net/neoforged/gradle/common/runtime/tasks/DecompilerExecute.java‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
package net.neoforged.gradle.common.runtime.tasks;
2+
3+
import net.neoforged.gradle.common.extensions.problems.IProblemReporter;
4+
import net.neoforged.gradle.dsl.common.tasks.Execute;
5+
import org.gradle.api.GradleException;
6+
import org.gradle.api.problems.Problems;
7+
import org.gradle.api.tasks.CacheableTask;
8+
import org.gradle.api.tasks.Internal;
9+
10+
import javax.inject.Inject;
11+
import java.io.ByteArrayOutputStream;
12+
import java.io.IOException;
13+
import java.io.OutputStream;
14+
15+
@CacheableTask
16+
public abstract class DecompilerExecute extends DefaultExecute
17+
{
18+
19+
private final OOMDetector detector = new OOMDetector();
20+
21+
@Override
22+
public OutputStream createErrorOutputStream()
23+
{
24+
return new BifurcatingOutputStream(
25+
super.createErrorOutputStream(),
26+
new OOMDetectorStream(this.detector)
27+
);
28+
}
29+
30+
@Internal
31+
public OOMDetector getDetector()
32+
{
33+
return detector;
34+
}
35+
36+
@Override
37+
public void doExecute() throws Exception
38+
{
39+
try {
40+
super.doExecute();
41+
} catch (Exception ex) {
42+
detectError(false);
43+
throw ex;
44+
}
45+
46+
detectError(true);
47+
}
48+
49+
private void detectError(boolean throwError)
50+
{
51+
if (getDetector().failed()) {
52+
//We failed, so we can access the project now, to get the reporter
53+
//We should not need to care about the config cache here,
54+
getProject().getExtensions().getByType(IProblemReporter.class)
55+
.reporting(problem -> {
56+
problem.contextualLabel("decompiler")
57+
.id("decompiler", "memory")
58+
.details("The Decompiler could not successfully decompile Minecraft because it ran out of memory. Modify your runtime configuration and the decompiler subsystems configuration, and try again.")
59+
.solution("Either increase the memory allowance for the decompiler, or reduce the concurrency on the decompiler to reduce memory consumption.")
60+
.section("common-decompiler-settings");
61+
}, getLogger());
62+
63+
if (throwError) {
64+
throw new GradleException("The decompiler failed to run. It ran out of memory.");
65+
}
66+
}
67+
}
68+
69+
public static final class OOMDetector {
70+
private final StringBuilder resultBuilder = new StringBuilder();
71+
72+
private OOMDetector() {
73+
}
74+
75+
private void addLog(String log) {
76+
resultBuilder.append(log);
77+
}
78+
79+
public boolean failed() {
80+
return this.resultBuilder.toString().contains("java.lang.OutOfMemoryError");
81+
}
82+
}
83+
84+
public static final class OOMDetectorStream extends OutputStream {
85+
private final OOMDetector detector;
86+
private final ByteArrayOutputStream collectionDelegate = new ByteArrayOutputStream();
87+
88+
public OOMDetectorStream(final OOMDetector detector) {this.detector = detector;}
89+
90+
@Override
91+
public void write(final int b) throws IOException
92+
{
93+
collectionDelegate.write(b);
94+
}
95+
96+
@Override
97+
public void flush() throws IOException
98+
{
99+
super.flush();
100+
collectionDelegate.flush();
101+
}
102+
103+
@Override
104+
public void close() throws IOException
105+
{
106+
super.close();
107+
collectionDelegate.close();
108+
109+
this.detector.addLog(collectionDelegate.toString());
110+
}
111+
}
112+
}

‎common/src/main/java/net/neoforged/gradle/common/runtime/tasks/RecompileSourceJar.java‎

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,6 @@ public abstract class RecompileSourceJar extends JavaCompile implements Runtime
4646
public RecompileSourceJar() {
4747
super();
4848

49-
//We use a custom instance here that marks the sourcepath as an incremental field, allowing us to provide the compiler
50-
//with all required elements directly while keeping incremental compile support for II.
51-
ReflectionUtils.setFinalFieldUnchecked(this, "compileOptions", getObjectFactory().newInstance(RecompileOptions.class));
52-
5349
arguments = getObjectFactory().newInstance(RuntimeArgumentsImpl.class, getProviderFactory());
5450
multiArguments = getObjectFactory().newInstance(RuntimeMultiArgumentsImpl.class, getProviderFactory());
5551

@@ -90,8 +86,8 @@ public RecompileSourceJar() {
9086
getOptions().setWarnings(false);
9187
getOptions().setVerbose(false);
9288
getOptions().setDeprecation(false);
93-
getOptions().setIncremental(true);
94-
getOptions().getIncrementalAfterFailure().set(true);
89+
getOptions().setIncremental(false);
90+
getOptions().getIncrementalAfterFailure().set(false);
9591

9692
setSource(getCompileFileRoot());
9793

@@ -230,25 +226,4 @@ public Provider<FileTree> getOutputAsTree()
230226
{
231227
return getOutput().map(it -> getArchiveOperations().zipTree(it));
232228
}
233-
234-
public static abstract class RecompileOptions extends CompileOptions {
235-
236-
@Inject
237-
public RecompileOptions(final ObjectFactory objectFactory)
238-
{
239-
super(objectFactory);
240-
}
241-
242-
@Incremental
243-
@Optional
244-
@IgnoreEmptyDirectories
245-
@PathSensitive(PathSensitivity.RELATIVE)
246-
@InputFiles
247-
@ToBeReplacedByLazyProperty
248-
@Override
249-
public @Nullable FileCollection getSourcepath()
250-
{
251-
return super.getSourcepath();
252-
}
253-
}
254229
}

‎common/src/main/java/net/neoforged/gradle/common/util/DependencyCollectorInjector.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package net.neoforged.gradle.common.util;
2+
3+
import org.gradle.api.artifacts.dsl.DependencyCollector;
4+
5+
import javax.inject.Inject;
6+
7+
public interface DependencyCollectorInjector
8+
{
9+
10+
@Inject
11+
public DependencyCollector dependencyCollector();
12+
}

‎common/src/main/java/net/neoforged/gradle/common/util/ReflectionUtils.java‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package net.neoforged.gradle.common.util;
22

3-
import java.lang.reflect.Field;
3+
import java.util.ArrayList;
4+
import java.util.Arrays;
5+
import java.util.List;
6+
import java.util.stream.Collectors;
47

58
public class ReflectionUtils {
69
/**
@@ -44,4 +47,25 @@ public static void setFinalFieldUnchecked(Object target, String fieldName, Objec
4447
throw new RuntimeException("Failed to set final field '" + fieldName + "'", e);
4548
}
4649
}
50+
51+
public static void setFinalFieldUncheckedWithAlternatives(Object target, Object value, String... fieldNames) {
52+
final List<ReflectiveOperationException> exceptions = new ArrayList<>(fieldNames.length);
53+
for (final String fieldName : fieldNames)
54+
{
55+
try {
56+
setFinalField(target, fieldName, value);
57+
return;
58+
} catch (ReflectiveOperationException e) {
59+
exceptions.add(e);
60+
}
61+
}
62+
63+
final RuntimeException ex = new RuntimeException("Failed to set final field '" + String.join(", ", fieldNames) + "'");
64+
for (final ReflectiveOperationException exception : exceptions)
65+
{
66+
ex.addSuppressed(exception);
67+
}
68+
69+
throw ex;
70+
}
4771
}

‎common/src/main/java/net/neoforged/gradle/common/util/ToolUtilities.java‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,13 @@
55
import net.neoforged.gradle.util.ModuleDependencyUtils;
66
import org.gradle.api.Project;
77
import org.gradle.api.artifacts.Configuration;
8-
import org.gradle.api.artifacts.ConfigurationContainer;
98
import org.gradle.api.artifacts.Dependency;
109
import org.gradle.api.artifacts.ResolvedArtifact;
11-
import org.gradle.api.artifacts.dsl.Dependencies;
1210
import org.gradle.api.artifacts.dsl.DependencyCollector;
13-
import org.gradle.api.artifacts.dsl.DependencyHandler;
1411
import org.gradle.api.artifacts.result.ResolvedArtifactResult;
15-
import org.gradle.api.file.RegularFileProperty;
1612
import org.gradle.api.provider.Provider;
1713

1814
import java.io.File;
19-
import java.util.List;
2015
import java.util.function.Function;
2116
import java.util.function.Supplier;
2217

@@ -41,7 +36,8 @@ public static Provider<File> resolveTool(final Project project, final Provider<S
4136
//If we were to use the provider directly, for example via map, the lambda would need to capture
4237
//the project that converts the string to a dependency.
4338
//This breaks the configuration cache as Projects can not be serialized.
44-
final DependencyCollector collector = project.getObjects().dependencyCollector();
39+
final DependencyCollectorInjector inject = project.getObjects().newInstance(DependencyCollectorInjector.class);
40+
final DependencyCollector collector = inject.dependencyCollector();
4541
collector.add(tool.map(project.getDependencies()::create));
4642
final Configuration config = ConfigurationUtils.temporaryUnhandledConfiguration(
4743
project.getConfigurations(),

0 commit comments

Comments
 (0)