Skip to content

Conversation

@rnro
Copy link
Contributor

@rnro rnro commented Apr 11, 2025

This PR upstreams the re-usable CMake workflow from the SwiftNIO repository. It in turn generalized and built on the existing CMake check scripts in swift-asn1, swift-certificates and swif-crypto.

The workflow performs two checks:

  • Ensure that the CMakeLists files in the repository are up-to-date
  • Perform a CMake build using ninja

@rnro rnro marked this pull request as ready for review April 11, 2025 20:29
@rnro rnro requested a review from a team as a code owner April 11, 2025 20:29
This commit upstreams the re-usable CMake workflow from the SwiftNIO
repository. It in turn generalized and built on the existing CMake check
scripts in swift-asn1, swift-certificates and swif-crypto.

The workflow performs two checks:
* Ensure that the `CMakeLists` files in the repository are up-to-date
* Perform a CMake build using `ninja`
@shahmishal
Copy link
Member

@rnro thank you!

Can you add a check in the .github/workflows/pull_request.yml to verify the CMake check?

@rnro
Copy link
Contributor Author

rnro commented Apr 22, 2025

@shahmishal What do you have in mind? Do you want something as heavyweight as setting up a test project which uses CMake?

@FranzBusch
Copy link
Member

@shahmishal What do you have in mind? Do you want something as heavyweight as setting up a test project which uses CMake?

I think a small example package that uses cmake should be enough e.g. swift package init and then adding the necessary cmake files

@shahmishal
Copy link
Member

The repository contains a few examples if that helps.

https://github.com/apple/swift-cmake-examples/

Copy link
Member

@etcwilde etcwilde left a comment

Choose a reason for hiding this comment

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

I'm pretty concerned about the update cmake lists scripts. I think that if this is being upstreamed into the swiftlang proper, it will need to work for all of the projects in the swiftlang umbrella, not just ones that look like a SwiftPM project.

I'm generally concerned about automatically trying to update CMakeLists files because CMake projects don't require the same amount of structure as a SwiftPM project. Not all of the sources in a CMake project need to be compiled in unconditionally. In the runtime build, we use target_sources to split out sources that are target-specific and that the build isn't building for. This script won't catch those cases and will unconditionally add those files to the target twice. There are also sources coming from other directories that are compiled into the targets that will get dropped by the script. There are sometimes more than one add_library invocation in a given CMakeLists file. Sometimes it's for object libraries, sometimes interface libraries, sometimes it's for something that is really meant to by a static library or shared library. In some cases, the source files must be put in a specific order to avoid crashing the compiler. I don't see this script handling any of those cases correctly.

Finally, what about Windows?

@shahmishal, how do we feel about using the GNU-find?

CC @compnerd, thoughts?

@FranzBusch
Copy link
Member

not just ones that look like a SwiftPM project.

What if we scope this down to exactly just that? We have a bunch of packages that also support Cmake based builds and that is what this workflow intends to solve.

@compnerd
Copy link
Member

Yeah, this is definitely not a good implementation with the use of perl and bash - can we rewrite this in Powershell to be more portable?

I think that we shouldn't be using the GNU tools - they are slow and do occasionally fail on Windows.

I fully agree with the concerns about the structure dependencies. For something to go into this repository, it should really be a fully generic solution. SPM doesn't have conditional compilation, so I'm not sure if there is a way to extract that information from SPM. Perhaps we should consider the opposite? Make CMake the ultimate truth for the build rules and generate the SPM package.swift?

@FranzBusch
Copy link
Member

I fully agree with the concerns about the structure dependencies. For something to go into this repository, it should really be a fully generic solution. SPM doesn't have conditional compilation, so I'm not sure if there is a way to extract that information from SPM. Perhaps we should consider the opposite? Make CMake the ultimate truth for the build rules and generate the SPM package.swift?

I don't think this is the place to litigate what the build system should be. The Swift package manager is the de-facto build system for the Swift ecosystem. The majority of packages are only capable of being build with it. The workflows in this repository are intended for packages that are part of the Swift project or closely related to it. Right now the focus is on providing workflows for projects that are developed as packages. The reality is that most projects right now are following a SwiftPM folder structure and generate the appropriate CMake files for it.

I don't think we should blow up this particular workflow to completely change the way we build and maintain our packages right now. The first step is to align what we do right now in each repo separately into this repo and evolve it from there.

Yeah, this is definitely not a good implementation with the use of perl and bash - can we rewrite this in Powershell to be more portable?

I think that we shouldn't be using the GNU tools - they are slow and do occasionally fail on Windows.

I think that's a fair point but only regarding the build script here. The cmake list generation script should be fine to just run on a Linux based runner since it only checks that the lists are up-to-date. For the build script we probably want to provide two variants. However, right now we are not running CMake based CI on Windows at all. So that's something we can add in a follow up PR to avoid expanding the scope of what this PR aims to do - Moving the shared workflow that each repo currently runs into a central place.

@compnerd
Copy link
Member

I think that's a fair point but only regarding the build script here. The cmake list generation script should be fine to just run on a Linux based runner since it only checks that the lists are up-to-date. For the build script we probably want to provide two variants. However, right now we are not running CMake based CI on Windows at all. So that's something we can add in a follow up PR to avoid expanding the scope of what this PR aims to do - Moving the shared workflow that each repo currently runs into a central place.

I disagree on this point. You are adding this to a generic repository that is shared and meant to support all the platforms. I believe that there already was some work to support Windows. The NIO project seemed uninterested in adding the support for Windows. I think that we could also leave the workflow there until we have the ability to expand this to be a generic solution.

@FranzBusch
Copy link
Member

I disagree on this point. You are adding this to a generic repository that is shared and meant to support all the platforms. I believe that there already was some work to support Windows. The NIO project seemed uninterested in adding the support for Windows. I think that we could also leave the workflow there until we have the ability to expand this to be a generic solution.

You are right that this repo intends to support all platforms and it already does. I think you missed my point here or maybe I'm missing something. The script that checks the cmake lists are up-to-date should produce the same results on all platforms right? That's why we only need to run it on one. That can be Linux, macOS, or Windows really. This is similar to other scripts that we have in here such as our format checking scripts which we only run on Linux runners. Not every script needs to run on every platform.

Now that's only for the cmake lists script. The cmake based build script should definitely also have a Windows version of it.

@rnro
Copy link
Contributor Author

rnro commented Apr 29, 2025

Thanks everyone for the thoughtful feedback on this.

I appreciate that the PR does not cover what is quite a broad problem space; broader than I initially appreciated. The script as it stands is useful for a few package-based Swift on Server repositories so I thought I would see if it was more widely useful. I don't have bandwidth to solve the general problem right now so I'll close the pull request for the time being.

@rnro rnro closed this Apr 29, 2025
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.

5 participants