Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for compilation databases #2962

Merged
merged 14 commits into from
Dec 3, 2019

Conversation

kastiglione
Copy link
Contributor

@kastiglione kastiglione commented Nov 14, 2019

This is to add support for compilation databases (aka compile_commands.json) as an alternative to parsing compiler invocations from a build log.

A compilation database can be passed using the newly added --compile-commands flag. This new flag is mutually exclusive to --compiler-log-path.

TODO

  • Add tests

@kastiglione
Copy link
Contributor Author

This is an early draft, I haven't added tests and some changes should be refactored.

@SwiftLintBot
Copy link

SwiftLintBot commented Nov 14, 2019

1 Warning
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 1.24s vs 1.24s on master (0% slower)
📖 Linting Alamofire with this PR took 2.18s vs 2.19s on master (0% faster)
📖 Linting Firefox with this PR took 8.78s vs 8.74s on master (0% slower)
📖 Linting Kickstarter with this PR took 14.18s vs 14.21s on master (0% faster)
📖 Linting Moya with this PR took 1.17s vs 1.19s on master (1% faster)
📖 Linting Nimble with this PR took 1.36s vs 1.37s on master (0% faster)
📖 Linting Quick with this PR took 0.53s vs 0.53s on master (0% slower)
📖 Linting Realm with this PR took 2.32s vs 2.32s on master (0% slower)
📖 Linting SourceKitten with this PR took 0.98s vs 0.98s on master (0% slower)
📖 Linting Sourcery with this PR took 6.62s vs 6.58s on master (0% slower)
📖 Linting Swift with this PR took 11.22s vs 11.19s on master (0% slower)
📖 Linting WordPress with this PR took 14.21s vs 14.27s on master (0% faster)

Generated by 🚫 Danger

@kastiglione
Copy link
Contributor Author

I don't see any tests related to --compiler-log-path. Is there any precedent for how to test this, or should I come up with something?

@PaulTaykalo
Copy link
Collaborator

@kastiglione
Well, In general, it is expected to have some fixtures with files we're about to parse in this type of functionality. Some small example will be enough. as well as the results we're about to expect after parsing

@jpsim
Copy link
Collaborator

jpsim commented Nov 27, 2019

This looks great, Dave! I wanted to take a stab at adding a test target for the swiftlint CLI executable, but it seems like SwiftPM doesn't support that 😞. I filed SR-11866 to track.

With this in mind, I'm ok merging this as-is without tests since that's (unfortunately) in line with the rest of what we have in Source/swiftlint.

Let me know when you're done with this and I can merge.

@kastiglione
Copy link
Contributor Author

Thanks for reviewing and checking into the tests. I'll convert this out of Draft and ping you when it's ready.

@kastiglione kastiglione marked this pull request as ready for review December 3, 2019 18:07
@kastiglione
Copy link
Contributor Author

Ready, thanks!

return nil
}

// Convert the compilation database to a dictionary, with source files as keys and compiler arguments as values.
Copy link
Contributor Author

@kastiglione kastiglione Dec 3, 2019

Choose a reason for hiding this comment

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

A note about this dictionary. It could use quite a lot of redundant memory for large modules. For each file in a module, there will be a single compiler invocation (which covers each file in the module). For example:

swiftc --yada --yada File1.swift File2.swift ... FileN.swift 

This command line will be copied N times, one for each path in the module. In theory, the same array could be shared for each file in the module. Array deduping (or string interning) would reduce the memory here.

Just wanted to make a note of this.

@jpsim jpsim merged commit 36e8d9f into realm:master Dec 3, 2019
@kastiglione kastiglione deleted the dl/add---compile-commands-flag branch December 3, 2019 18:42
@PaulTaykalo
Copy link
Collaborator

Just curious, @kastiglione How are you going to receive compilation database files?
I was wondering if there's an option to generate those for testing this functionality

@kastiglione
Copy link
Contributor Author

kastiglione commented Dec 3, 2019

We have a tool to generate compilation databases for Bazel projects. I would like to open source it, but even then it's use is limited to Bazel projects.

It's doable to generate a compilation database from an xcodebuild log. The json structure is straight forward.

@jpsim
Copy link
Collaborator

jpsim commented Dec 3, 2019

Talking to Dave, it occurred to us that if we standardized SourceKitten/SwiftLint to use the compilation database format as input whenever we need to deal with SourceKit compiler arguments, that could simplify and standardize working with a variety of build tools, such as Xcode, bazel, SwiftPM and CMake.

@acecilia
Copy link
Contributor

👋 @kastiglione I see the compilation database json that swiftlint takes should have an array of dictionaries with file and arguments, but the usual compilation database contains file, directory, and command. Do you know why the difference?

@keith
Copy link
Collaborator

keith commented Dec 7, 2022

@acecilia i don't recall the history here, but i imagine we could make both work if you have another tool that does the traditional format. probably just have to be careful about space escapes

@acecilia
Copy link
Contributor

Thanks. I end up adapting my tool to the existing format

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants