-
Notifications
You must be signed in to change notification settings - Fork 134
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 all 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 { | ||
|
||
| // 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]) { | ||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
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
|
||||||||||||||||||||||||||||||||||||||||||
| 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 | |
| } | |
| notices = mergeNotices(parentNotices: notices, subNotices: subNotices, key: "warnings") | |
| notices = mergeNotices(parentNotices: notices, subNotices: subNotices, key: "errors") | |
| notices = mergeNotices(parentNotices: notices, subNotices: subNotices, key: "notes") |
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.
This is a good advice. I'd also extract the logic into a method since the method is getting very deeply nested.
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
RealTestsfolder, 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.