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

change gradle task syntax and placement #1657

Merged
merged 11 commits into from
May 23, 2024
Merged

Conversation

munkiki7
Copy link
Contributor

@munkiki7 munkiki7 commented May 21, 2024

Reason for changes: random android smoke tests failures during CI tasks.

We have noticed that sometimes the upload task is executed before the assemble task is finished, which resulted in not all .so files being generated at the moment it was executed. After that the uploaded symbols validation was failing.

Now we are attaching the symbol upload task to either assembleDebug or assembleRelease tasks of the launcher build.gradle instead of the root build.gradle file.

One of the other advantages is that with the previous approach the symbol upload task was executed during the gradle sync process in the Android Studio (if the project was exported), as it was bound to the last task being executed (even though it was not a build-related task).

Previous syntax:

gradle.taskGraph.whenReady { taskGraph ->
    gradle.taskGraph.allTasks[-1].doLast { task ->
        //task body
    }
}

New syntax (needs to be wrapped in the afyterEvaluate call, because we have to wait until the task list is built):

afterEvaluate {
    task sentryUploadSymbols {
        doLast {
            //task body
        }
    }
    
    tasks.assembleDebug.finalizedBy sentryUploadSymbols
    tasks.assembleRelease.finalizedBy sentryUploadSymbols
}

@munkiki7 munkiki7 marked this pull request as ready for review May 22, 2024 10:46
@@ -53,8 +54,8 @@ private string _symbolUploadTaskFormat
stringBuilder.AppendLine($" def sentryLogFile = new FileOutputStream('{logsDir}/sentry-symbols-upload.log')");
}
stringBuilder.AppendLine(" exec {");
stringBuilder.AppendLine(" environment 'SENTRY_PROPERTIES', './sentry.properties'");
stringBuilder.AppendLine($" executable '{SentryCliMarker}'");
stringBuilder.AppendLine(" environment 'SENTRY_PROPERTIES', file(\"${rootDir}/sentry.properties\").absolutePath");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the change to the path coming from?

@@ -81,7 +86,7 @@ private string _symbolUploadTaskFormat

_unityProjectPath = unityProjectPath;
_gradleProjectPath = gradleProjectPath;
_gradleScriptPath = Path.Combine(_gradleProjectPath, "build.gradle");
_gradleScriptPath = Path.Combine(_gradleProjectPath, "launcher/build.gradle");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Because the SDK now modifies the launcher/build.gradle. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task is now added to the topmost project - launcher that depends on unityFramework target.

We are now executing the symbol upload task after the assembleDebug or assembleRelease tasks, which do not exist in the scope of the build.gradle file we were modifying previously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of messing with the path, can we just drop the cli executable and the .properties next to the launcher gradle file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That depends on whether anything else is also using the locations of those files. But we can check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sentry CLI is the one looking for the .properties so I think it's safe to keep all that in once place.

@bitsandfoxes
Copy link
Contributor

Thanks for tackling this!

CHANGELOG.md Outdated Show resolved Hide resolved
@bitsandfoxes bitsandfoxes merged commit 2e0df88 into main May 23, 2024
122 checks passed
@bitsandfoxes bitsandfoxes deleted the fix/change-gradle-syntax branch May 23, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants