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

Support pre-generated Swift symbolgraph files #1294

Merged
merged 4 commits into from Nov 30, 2021

Conversation

esteluk
Copy link
Contributor

@esteluk esteluk commented Nov 29, 2021

Since Swift 5.5, the Swift compiler has supported the -emit-symbol-graph
and -emit-symbol-graph-dir flags to directly create Symbol graph files
as part of the compilation process. If a developer has access to these
files, it's not necessary for Jazzy to use symbolgraph-extract to attempt
to separately generate them.

This commit adds a --symbolgraph_directory configuration option to Jazzy.
If this option is provided, Jazzy skips the generation of symbolgraph files
and directly parses any *.symbol.json files in the given directory.

  • Check if any updates need to be made to test specs

Since Swift 5.5, the Swift compiler has supported the `-emit-symbol-graph`
and `-emit-symbol-graph-dir` flags to directly create Symbol graph files
as part of the compilation process. If a developer has access to these
files, it's not necessary for Jazzy to use `symbolgraph-extract` to attempt
to separately generate them.

This commit adds a `--symbolgraph_directory` configuration option to Jazzy.
If this option is provided, Jazzy skips the generation of symbolgraph files
and directly parses any `*.symbol.json` files in the given directory.
@esteluk esteluk changed the title WIP: Support pre-generated Swift symbolgraph files Support pre-generated Swift symbolgraph files Nov 29, 2021
@esteluk
Copy link
Contributor Author

esteluk commented Nov 29, 2021

Sorry, meant to raise this initially on my fork rather than here, but having done so it might as well stay open...

I found this a valuable change because I was struggling a little bit to find the correct build-tool-arguments that would generate valid symbol files for my framework, particularly when using the Simulator SDK. Once I realised that the build process had already spit out the required files, it seemed like it would be useful for Jazzy to be able to consume them directly.

If this is a direction that you'd prefer not to go down, I'd be happy to close the PR.

CHANGELOG.md Outdated
@@ -9,7 +9,8 @@

##### Enhancements

* None.
* Support using pre-generated symbolgraph files in Swift symbolgraph mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need end-of-line spaces for markdown, see CONTRIBUTING.md.

README.md Outdated
5. With pre-generated symbolgraph files:
```shell
jazzy --module MyMod --swift-build-tool symbolgraph
--symbolgraph_directory Build/symbolgraphs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be --symbolgraph-directory here? With dash not underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sorry, I'd been staring at the yaml files too much!

README.md Outdated
```
If you've separately generated symbolgraph files by the use of
`-emit-symbol-graph`, you can pass the location of these files using
`--symbolgraph_directory` from where they can be parsed directly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(same)

@@ -69,6 +67,20 @@ def self.arguments(config, output_path)
args + config.build_tool_arguments
end

# Parse the symbol files in the given directory
def self.parseSymbols(directory)
Copy link
Collaborator

Choose a reason for hiding this comment

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

snake-case for Ruby, self.parse_symbols

@@ -238,6 +240,19 @@ def configure_cocoapods
'--swift-build-tool symbolgraph ' \
"--build-tool-arguments -I,#{module_path} "
end

describe 'Creates docs for Swift project from symbolgraph files' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to not change this file, it's quite a lot of overhead to get this working and adding a entire new output set adds change-review burden in the future as well. I appreciate the effort to add some kind of test for the feature but jazzy doesn't have a way of doing convenient unit-testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, that makes sense. I was hoping that it would be possible to reuse the same before/after states as the main symbolgraph tests, but I think there are some differences in output that would make this impossible. Giving it half-of another shot, but remove this test shortly otherwise.

@johnfairh
Copy link
Collaborator

I'm happy to merge this option -- left a few comments. I don't know what's up with Danger in CI, maybe we have something misconfigured for forks, but you should be able to run rake rubocop locally. Don't worry if Danger doesn't sort itself out on your next commit, I'll take care of it.

esteluk and others added 3 commits November 29, 2021 19:03
* Conform to contribution guidelines
* Use correct command line arguments in README examples
* Use snake_case for method name
Fix Rubocop errors

Reorder jobs so at least Rubocop runs on forks
@johnfairh
Copy link
Collaborator

Turns out Danger + GitHub actions is broken on forks due to secret handling.

@johnfairh johnfairh merged commit 819ae6f into realm:master Nov 30, 2021
@johnfairh
Copy link
Collaborator

All done - thanks for the PR!

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.

None yet

2 participants