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

Make build plugin communicate files via a file list rather than space-separated CLI arguments #10

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

dfed
Copy link
Owner

@dfed dfed commented Jan 11, 2024

This PR speeds up the SafeDI plugin by using a .csv file to transfer the file list to the plugin rather than sending the file list over as a very long CLI argument.

This code is tested via the Build Project Integration on Xcode 15 job.

I removed the option to pass in an array of swift file paths to the SafeDITool, which makes this change a breaking one.

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4274e6f) 97.66% compared to head (d17ca78) 97.63%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #10      +/-   ##
==========================================
- Coverage   97.66%   97.63%   -0.03%     
==========================================
  Files          37       37              
  Lines        7322     7324       +2     
==========================================
  Hits         7151     7151              
- Misses        171      173       +2     
Files Coverage Δ
Sources/SafeDITool/SafeDITool.swift 44.04% <0.00%> (-0.54%) ⬇️

@dfed dfed self-assigned this Jan 11, 2024
@dfed dfed marked this pull request as ready for review January 11, 2024 19:27
)
.write(toPath: inputSwiftFilesFilePath)
let arguments = [
"--swift-file-paths-file-path",
Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't love this argument name. If you've got better ideas, I am all ears.

Copy link
Collaborator

@MrAdamBoyd MrAdamBoyd Jan 12, 2024

Choose a reason for hiding this comment

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

  • --swift-sources-file-path
  • --swift-source-files-file-path
  • --swift-source-list-file-path

Best I got

Copy link
Owner Author

Choose a reason for hiding this comment

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

ooooh swiftSources is good.

@@ -112,3 +133,17 @@ extension SafeDIGenerateDependencyTree: XcodeBuildToolPlugin {
}
}
#endif

extension Data {
fileprivate func write(toPath filePath: String) throws {
Copy link
Owner Author

Choose a reason for hiding this comment

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

This extension also exists in another module, but this plug in module cannot depend on any other library we have in this package

Comment on lines +139 to +148
#if os(Linux)
try write(to: URL(fileURLWithPath: filePath))
#else
if #available(macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0, *) {
try write(to: URL(filePath: filePath))
} else {
try write(to: URL(fileURLWithPath: filePath))
}
#endif
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can't be simplified?

         if #available(macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0, *) {
             try write(to: URL(filePath: filePath))
         } else {
             try write(to: URL(fileURLWithPath: filePath))
         }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah nvm, it's #if something not if #something

Copy link
Owner Author

Choose a reason for hiding this comment

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

yup. tried that 🙂. Took me awhile to get this right. Linux availability support is... bad (read: nonexistent).

@dfed
Copy link
Owner Author

dfed commented Jan 12, 2024

I'll merge this at the same time as #8 so we can roll a single breaking change.

@dfed
Copy link
Owner Author

dfed commented Jan 12, 2024

Merging this, but @MrAdamBoyd if you have more feedback on what I pushed after you reviewed let me know – happy to address post-merge. I won't be releasing for a few days so we have time.

@dfed dfed merged commit 484ee1e into main Jan 12, 2024
5 checks passed
@dfed dfed deleted the dfed--small-arg-list branch January 12, 2024 23:02
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.

2 participants