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

[local-build-plugin][build-tools][eas-build-job] copy Xcode build logs to target destination on local iOS build failure to simplify debuging #447

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

szdziedzic
Copy link
Member

@szdziedzic szdziedzic commented Oct 14, 2024

Why

Usually, the most useful info for iOS build failures is present within Xcode build logs. They are always uploaded for cloud builds and are easy to access there. When it comes to local builds they are not copied to the target destination by default and are deleted together with temp working dir. One needs to use EAS_LOCAL_BUILD_SKIP_CLEANUP=1 to prevent this behavior in the next build and inspect Xcode logs then, to be able to learn what happened.

This seems unintuitive for some users who think RUN_FASTLANE logs are the source of truth. It's also kind of inconvenient.

How

When the iOS local build fails always move Xcode logs to the target destination to save users work and make it easier to see the root cause of an issue.

Test Plan

Test locally.

Copy link
Member Author

@szdziedzic szdziedzic marked this pull request as ready for review October 14, 2024 06:38
Copy link
Contributor

@radoslawkrzemien radoslawkrzemien left a comment

Choose a reason for hiding this comment

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

LGTM 🌔

Comment on lines +35 to +40
): artifactSpec is TSpec & { type: ManagedArtifactType } => {
if (Object.values(ManagedArtifactType).includes(artifactSpec.type as ManagedArtifactType)) {
return true;
}
return false;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

): artifactSpec is TSpec & { type: ManagedArtifactType } => Object.values(ManagedArtifactType).includes(artifactSpec.type as ManagedArtifactType)

?

…s to target destination on local iOS build failure to simplify debuging
@szdziedzic szdziedzic force-pushed the 10-14-_local-build-plugin_build-tools_eas-build-job_copy_xcode_build_logs_to_target_destination_on_local_ios_build_failure_to_simplify_debuging branch from 73b604c to b14f074 Compare October 17, 2024 14:01
@szdziedzic szdziedzic requested a review from sjchmiela October 17, 2024 14:01
if (isManagedArtifact(artifact)) {
if (
artifact.type === ManagedArtifactType.XCODE_BUILD_LOGS &&
artifact.runStatus !== 'errored'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to just always upload Xcode build logs? It would simplify code greatly (no tracking of runStatus, uploading artifact in finally)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it would. Would it be ok for you as a user if they were always uploaded for you local builds (copied to project dir) 🤔

l?.info({ phase: 'PREPARE_ARTIFACTS' }, `Writing application archive to ${destPath}`);
} else if (artifactType === ManagedArtifactType.BUILD_ARTIFACTS) {
l?.info({ phase: 'PREPARE_ARTIFACTS' }, `Writing build artifacts to ${destPath}`);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer switch over enums. This way if we add more managed types we won't forget to add handling.

? `build-${Date.now()}${suffix}`
: artifactType === ManagedArtifactType.BUILD_ARTIFACTS
? `artifacts-${Date.now()}${suffix}`
: `xcode-logs-${Date.now()}${suffix}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as below

@szdziedzic szdziedzic marked this pull request as draft November 5, 2024 11:42
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