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

build: add an initial CMake based build system for DocC #818

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Feb 6, 2024

Using this (along with its dependencies) allows us to significantly reduce the build times for the Swift toolchain (~7% of the overall build time in local testing).

See https://swift.org/LICENSE.txt for license information
#]]

add_library(SwiftDocC
Copy link
Contributor

Choose a reason for hiding this comment

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

Does these lists need to be maintained whenever we add a new file to either of these targets?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is the general pattern for CMake - you manage the source list explicitly so that the build system knows what is part of the build and what is not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to specify directories instead of files lists? I'm concerned about the maintenance burden of keeping this list up to date.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not without basically making the build system brittle and unreliable. CI would easily catch any files removed. Any file that the build doesn't catch where the list is not maintained would imply that it can be dropped because there is no reference to the types/functions it defines.

@compnerd compnerd marked this pull request as ready for review November 15, 2024 18:03
@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

@swift-ci please test

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

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

This is how I understand the implications of these changes:

  • Using CMake instead of Swift Package Manager doesn't enable new functionality (for example building on new platforms) but it offers faster builds.
  • The CMake files require constant maintenance to work and will break if a new Swift source file is missing from the file list.
  • To catch issues of a missing file in the CMake file list, a new CI needs to run the CMake build.

Based on that understanding I inspected the SwitDocC git history. The numbers below were computed when 3cfeeb64b96709ca284d845637c75f2f8c248f32 was the latest commit on main.


In the last 30 days there was 20 new commit on main. These commits added or removed 18 files (13 added, 5 removed). In those 30 days there were 21 workdays. This means that on average over the last 30 days there was a new commit approximately every 1.05 days and on average a file added or removed on average every 1.17 days.

This means that if the CMake file list needed to be updated for every added or removed file, the data from the last 30 days indicate that it would need to be updated on average every 1.17 days.

However, about half of the added or removed files were tests, which AFAICT the CMake configuration doesn't try to build. There were 8 added or removed files in the "Sources" directory (6 added 2 removed) over the last 30 days. This means that there was a file added to or removed from the "Sources" directory on average every 2.63 days over the last 30 days. This data from the last 30 days indicate that the CMake file list would need to be updated on average every 2.63 days (almost twice a week).

If we instead expand this analysis to all the commits since February 6, when this PR was first opened, there were 175 commits, 108 added or removed files, 56 added or removes files in the "Sources" directory, and 205 workdays.

In this time period there was a new commit on average every 1.17 days and a file was added to or removed from the "Sources" directory on average every 3.66 days. This data since February 6 indicate that the CMake file list would need to be updated on average every 3.66 days (almost 3 times in two weeks).


These are just estimates but the actual number is probably somewhere in this ballpark; the CMake file list would need to be updated about once or twice every week (around 32%-40% of all commits) ongoing for as long as DocC is developed. This IMO is a rather frequent maintenance burden.

~7% faster toolchain builds is not insignificant but it's a benefit that only applies to people who use CMake and the ongoing maintenance burden of keeping the file list up-to-date applies to everyone, whether or not they use CMake to build DocC.

Unless there's some broader project goal that I'm not aware of here, in my very personal opinion I don't feel that this benefit is worth the associated cost.

My concerns would be alleviated if the CMake file list was automatically keep up-to-date.

My concerns would be lessened to a point where the benefits might outweigh the costs if:

  • it was possible to easily regenerate the CMake file list based on find Sources/SwiftDocC -type f -name "*.swift" | sort or something similar
  • there was some automatic validation that the CMake file list was up-to-date (for example a CI that builds using CMake or a new verification step in bin/tests that verify the file list contents)
  • "CONTRIBUTING.md" contained information about how to update the CMake file list when adding a new file that we could refer new contributors to.

CMakeLists.txt Outdated Show resolved Hide resolved
@compnerd
Copy link
Member Author

Using CMake instead of Swift Package Manager doesn't enable new functionality (for example building on new platforms) but it offers faster builds.

This is not entirely correct. It does enable new functionality - CMake has a broad platform support and bringing up Swift on new platforms is made easier by having the CMake based build. It allows bootstrapping without cross-compiling which is not always possible (e.g. I cannot use the macOS SDK on non-Apple hardware, Windows SDK is not available on macOS, etc). We currently are unable to ship DocC on Windows ARM64 due to the lack of CMake support.

The CMake files require constant maintenance to work and will break if a new Swift source file is missing from the file list.

The "constant maintenance" feels unnecessarily critical. It does require that the source list is maintained (this information is used for ensuring that build targets are properly rebuilt).

To catch issues of a missing file in the CMake file list, a new CI needs to run the CMake build.

We could change the CI to only build with CMake, and that would change this to "a new CI needs to run the SPM build".

Using this (along with its dependencies) allows us to significantly
reduce the build times for the Swift toolchain (~7% of the overall build
time in local testing).
@matthewbastien
Copy link
Member

I'd also like to add that this is currently blocking the documentation preview support in the VS Code Swift extension. The current approach is to have SourceKit-LSP handle documentation requests from the editor since it is the source of truth for all symbol related requests. However, SourceKit-LSP has a cmake build on Windows and, in order for it to use SwiftDocC as a dependency, requires that SwiftDocC also build with cmake on Windows.

I can take another approach of creating a new executable (e.g. doccd or some other appropriate name) that can communicate bi-directionally with SourceKit-LSP using some protocol like JSON-RPC, but that feels like a lot of duplicated code between the two and also doesn't solve the problem that docc isn't available on Windows arm64.

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.

3 participants