-
Notifications
You must be signed in to change notification settings - Fork 135
Fix C++ warnings incorrectly classified as errors #231
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
667aae3
36d6989
ac2d161
50c6509
70cbb31
34612eb
e9ff458
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,3 +14,4 @@ DerivedData/ | |
|
||
# Xcode 11 | ||
.swiftpm/ | ||
RealTests/ | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,17 @@ | |
let reporterOutput = ReporterOutputFactory.makeReporterOutput(path: options.outputPath) | ||
let logReporter = options.reporter.makeLogReporter() | ||
try logReporter.report(build: buildSteps, output: reporterOutput, rootOutput: options.rootOutput) | ||
|
||
// Exit with appropriate code based on build status | ||
if shouldExitWithFailureCode(buildSteps: buildSteps) { | ||
exit(1) | ||
} | ||
} | ||
|
||
private func shouldExitWithFailureCode(buildSteps: BuildStep) -> Bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] The exit logic based on a lowercased build status string may be brittle if more status values are introduced. Consider clarifying or centralizing build status definitions to make the check more robust. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
// Check if the main build failed | ||
let buildStatus = buildSteps.buildStatus.lowercased() | ||
return buildStatus.contains("failed") || buildStatus.contains("error") | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -98,6 +98,31 @@ | |||||||||||||||||||||||||||||||||||||||||
self.truncLargeIssues = truncLargeIssues | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
/// Collects all warnings and errors from a BuildStep tree and deduplicates them | ||||||||||||||||||||||||||||||||||||||||||
/// - parameter buildStep: The root BuildStep to collect notices from | ||||||||||||||||||||||||||||||||||||||||||
/// - returns: A tuple of (warnings, errors) arrays with duplicates removed | ||||||||||||||||||||||||||||||||||||||||||
private func collectAndDeduplicateNotices(from buildStep: BuildStep) -> ([Notice], [Notice]) { | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not obvious from the caller what the items in the tuple represent:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
var allWarnings: [Notice] = [] | ||||||||||||||||||||||||||||||||||||||||||
var allErrors: [Notice] = [] | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
func collectNotices(from step: BuildStep) { | ||||||||||||||||||||||||||||||||||||||||||
if let warnings = step.warnings { | ||||||||||||||||||||||||||||||||||||||||||
allWarnings.append(contentsOf: warnings) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
if let errors = step.errors { | ||||||||||||||||||||||||||||||||||||||||||
allErrors.append(contentsOf: errors) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
for subStep in step.subSteps { | ||||||||||||||||||||||||||||||||||||||||||
collectNotices(from: subStep) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
collectNotices(from: buildStep) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
return (allWarnings.removingDuplicates(), allErrors.removingDuplicates()) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
/// Parses the content from an Xcode log into a `BuildStep` | ||||||||||||||||||||||||||||||||||||||||||
/// - parameter activityLog: An `IDEActivityLog` | ||||||||||||||||||||||||||||||||||||||||||
/// - returns: A `BuildStep` with the parsed content from the log. | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -106,8 +131,12 @@ | |||||||||||||||||||||||||||||||||||||||||
buildStatus = BuildStatusSanitizer.sanitize(originalStatus: activityLog.mainSection.localizedResultString) | ||||||||||||||||||||||||||||||||||||||||||
let mainSectionWithTargets = activityLog.mainSection.groupedByTarget() | ||||||||||||||||||||||||||||||||||||||||||
var mainBuildStep = try parseLogSection(logSection: mainSectionWithTargets, type: .main, parentSection: nil) | ||||||||||||||||||||||||||||||||||||||||||
mainBuildStep.errorCount = totalErrors | ||||||||||||||||||||||||||||||||||||||||||
mainBuildStep.warningCount = totalWarnings | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// Collect and deduplicate all warnings and errors from the entire build tree | ||||||||||||||||||||||||||||||||||||||||||
let (deduplicatedWarnings, deduplicatedErrors) = collectAndDeduplicateNotices(from: mainBuildStep) | ||||||||||||||||||||||||||||||||||||||||||
mainBuildStep.errorCount = deduplicatedErrors.count | ||||||||||||||||||||||||||||||||||||||||||
mainBuildStep.warningCount = deduplicatedWarnings.count | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
mainBuildStep = decorateWithSwiftcTimes(mainBuildStep) | ||||||||||||||||||||||||||||||||||||||||||
return mainBuildStep | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -131,7 +160,41 @@ | |||||||||||||||||||||||||||||||||||||||||
targetErrors = 0 | ||||||||||||||||||||||||||||||||||||||||||
targetWarnings = 0 | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
let notices = parseWarningsAndErrorsFromLogSection(logSection, forType: detailType) | ||||||||||||||||||||||||||||||||||||||||||
var notices = parseWarningsAndErrorsFromLogSection(logSection, forType: detailType) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// For Swift compilations and other compilation types, also check subsections for errors/warnings | ||||||||||||||||||||||||||||||||||||||||||
// Also ensure Swift file-level compilations are processed correctly | ||||||||||||||||||||||||||||||||||||||||||
if (detailType == .swiftCompilation || detailType == .cCompilation || detailType == .other) && !logSection.subSections.isEmpty { | ||||||||||||||||||||||||||||||||||||||||||
// Initialize notices if nil | ||||||||||||||||||||||||||||||||||||||||||
if notices == nil { | ||||||||||||||||||||||||||||||||||||||||||
notices = ["warnings": [], "errors": [], "notes": []] | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
for subSection in logSection.subSections { | ||||||||||||||||||||||||||||||||||||||||||
if let subNotices = parseWarningsAndErrorsFromLogSectionAsSubsection(subSection, forType: detailType) { | ||||||||||||||||||||||||||||||||||||||||||
// Merge subsection notices with parent section notices | ||||||||||||||||||||||||||||||||||||||||||
if let parentWarnings = notices?["warnings"], let subWarnings = subNotices["warnings"] { | ||||||||||||||||||||||||||||||||||||||||||
notices?["warnings"] = parentWarnings + subWarnings | ||||||||||||||||||||||||||||||||||||||||||
} else if let subWarnings = subNotices["warnings"] { | ||||||||||||||||||||||||||||||||||||||||||
notices?["warnings"] = subWarnings | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
if let parentErrors = notices?["errors"], let subErrors = subNotices["errors"] { | ||||||||||||||||||||||||||||||||||||||||||
notices?["errors"] = parentErrors + subErrors | ||||||||||||||||||||||||||||||||||||||||||
} else if let subErrors = subNotices["errors"] { | ||||||||||||||||||||||||||||||||||||||||||
notices?["errors"] = subErrors | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
if let parentNotes = notices?["notes"], let subNotes = subNotices["notes"] { | ||||||||||||||||||||||||||||||||||||||||||
notices?["notes"] = parentNotes + subNotes | ||||||||||||||||||||||||||||||||||||||||||
} else if let subNotes = subNotices["notes"] { | ||||||||||||||||||||||||||||||||||||||||||
notices?["notes"] = subNotes | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+177
to
+193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The merging logic for 'warnings', 'errors', and 'notes' is repeated for each key. Consider extracting this duplication into a helper function to improve code maintainability.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good advice. I'd also extract the logic into a method since the method is getting very deeply nested. |
||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
let warnings: [Notice]? = notices?["warnings"] | ||||||||||||||||||||||||||||||||||||||||||
let errors: [Notice]? = notices?["errors"] | ||||||||||||||||||||||||||||||||||||||||||
let notes: [Notice]? = notices?["notes"] | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -146,6 +209,7 @@ | |||||||||||||||||||||||||||||||||||||||||
totalWarnings += warnings.count | ||||||||||||||||||||||||||||||||||||||||||
targetWarnings += warnings.count | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
var step = BuildStep(type: type, | ||||||||||||||||||||||||||||||||||||||||||
machineName: machineName, | ||||||||||||||||||||||||||||||||||||||||||
buildIdentifier: self.buildIdentifier, | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -310,6 +374,14 @@ | |||||||||||||||||||||||||||||||||||||||||
"errors": notices.getErrors(), | ||||||||||||||||||||||||||||||||||||||||||
"notes": notices.getNotes()] | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
private func parseWarningsAndErrorsFromLogSectionAsSubsection(_ logSection: IDEActivityLogSection, forType type: DetailStepType) | ||||||||||||||||||||||||||||||||||||||||||
-> [String: [Notice]]? { | ||||||||||||||||||||||||||||||||||||||||||
let notices = Notice.parseFromLogSection(logSection, forType: type, truncLargeIssues: truncLargeIssues, isSubsection: true) | ||||||||||||||||||||||||||||||||||||||||||
return ["warnings": notices.getWarnings(), | ||||||||||||||||||||||||||||||||||||||||||
"errors": notices.getErrors(), | ||||||||||||||||||||||||||||||||||||||||||
"notes": notices.getNotes()] | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
private func decorateWithSwiftcTimes(_ mainStep: BuildStep) -> BuildStep { | ||||||||||||||||||||||||||||||||||||||||||
swiftCompilerParser.parse() | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -395,4 +467,4 @@ | |||||||||||||||||||||||||||||||||||||||||
andCompilationDuration: lastStep.compilationEndTimestamp - app.startTimestamp) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'll remove it. What I did is I gathered a few hundred xcactivitylogs from DerivedData, put them in that
RealTests
folder, along with theirLogStoreManifest.plist
. Then had a script that ran XCLogParser on them and compared the # of warnings and errors. I then iterated on that to get it to match as much as possible. I'd need to check the xcactivitylogs to see if there's private info in there (like my global env vars). It started with 80% match and I got to 99.1% match.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't manually reviewed the code yet like I did for this PR (I had Claude Code iterate on that loop to increase the % of matching), but I just pushed the changes done to reach 99.1% on my local xcactivitylogs here: master...lapfelix:XCLogParser:feature/fine-tuned-warnings-to-match-xcode
I can review and clean up what's in there, but I do think there's value in testing real xcactivitylogs against what's in
LogStoreManifest.plist
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just had a standardized machine-readable format from Apple... 🤦🏼
That sounds fair! I trust you. I'd do a bit of clean up of the code and get an extra review and I'd say we are ready to merge.