I’m looking to fix #25864 and I was hoping to get my bearings. Just twothree questions to start with:
from looking at the code it appears that if I want to test the Xcode generator I need to use that generator to build CMake; is that right?
The basic problem here is that for this multi-config generator we need to incorporate the build configuration into the include paths referencing dependency Swift modules that are being built. Is there a pattern I should follow for this kind of thing? It seems like the sort of thing that must come up in other contexts, e.g. for link paths.
Is there anyone who can give general advice about working on this generator? I’m going to be dealing with a 700-line function right off the bat, so if you can save me from putting a foot wrong, it would be much appreciated.
Crickets. A little discouraging, but that’s OK; in case someone is listening now…
Looking at the existing tests for Swift, most things are only tested with Ninja. It seems like the first step is to get as many of those test cases as possible to work with Xcode. Now, it appears that lots of those tests are simply checking that the right dependencies are set up by the Ninja generator. For example, there are tests that check that no work is done when a 2nd build is invoked with no intervening changes. It seems to me that given the flexibility of Ninja to set up arbitrary dependencies, that makes sense, but (I think) the nature of Xcode projects is that any target that includes a source automatically gets a dependency on that source, and our generator really has no control over things like this. So my plan is to simply check that the initial builds of these example projects work with Xcode, and leave all the dependency graph checking as Ninja-only (with a comment containing this rationale).
I’d love to hear feedback about this plan, even “makes sense to me.”
You may be looking only at RunCMake.Swift, which does have a lot of Ninja-specific cases.
The Swift* tests defined here run with both Xcode and Ninja, and one only runs with Xcode. They all try building actual projects and running the resulting binaries.
So lines like this one could suppress the bug I’m trying to fix, if the libraries in the subdirectories were ever imported. I don’t think that line is there for any good reason, but there’s no comment. Any objection if I remove it?
I do have a specific bug with an issue tracker entry; it is linked from the first message in this thread. @compnerd is a good friend and I have called his attention to the issue and this thread, FWIW.
Looking at that file it is full of very specific conditions that CMake wouldn’t encounter unless the developer had special knowledge and requirements, such as explicit policy settings, setting that property, and forcing shared library builds (for reasons that appear to be outdated—I am successfully using static Swift libraries on Windows). My concern is that the reasons for these settings are largely unstated and nothing is testing a straightforward project. In fact, I have a fix for the problem that works on my simple example project but when I transplant that structure into the project being tested, the build still fails. Obviously I want to fix the problem even for this more “exotic” configuration, but I hope you see the problem I’m pointing to.
One could refactor the complex parts of the SwiftOnly test into other dedicated tests named for their purpose. Then the SwiftOnly test can serve as the simple case, as we do for COnly and CxxOnly.
Separate question: is it normal that when I use the Xcode generator to build CMake, any invocation of cmake --build rebuilds everything, every time? That’s making work on this unbelievably frustrating.
The particular case being called out here was to ensure that the property was considered when adding all targets. It should implicitly apply a -I flag to all Swift targets. This behavioural aspect should continue to be tested, so it would make sense to refactor the tests to ensure that we do not regress the CMAKE_Swift_MODULE_DIRECTORY handling for Swift targets.
OK, but as far as I can tell the test doesn’t validate that at all, as evidenced by the fact that the test passes even if I remove that line. Although I agree that should be tested I don’t think I want to be responsible for figuring out how to actually do it
There are a number of other things in that test that likewise shouldn’t be required for a Swift project to work, and I want to know if/how they should be refactored into tests distinct from all the general Swift testing, or removed:
Hmm, I wonder if this is a “regression” by progression. We got better about the dependency management for Swift code - and what previously would’ve failed no longer does.
I agree that fixing this is beyond the scope of what you are doing, but I don’t think that just dropping it is fair either. We should be able to isolate a single separate test for this.
Actually, no. If you read the docs, it talks about where module files are generated, rather than where they’re imported from.
In the example we’re discussing, subdirectories are added before that variable is set, which should mean the variable doesn’t affect those subdirectories and if a target declared after the variable were set linked only to SubA, it would be built with -I /directory/of/SubA.swiftmodule and not -I ${Swift_MODULE_DIRECTORY}.
So all you need to do to make the test work is to validate that one of the libraries declared after the variable is set actually gets its file generated into the specified Swift_MODULE_DIRECTORY. This seems to work with Ninja, but it fails with Xcode… because Xcode isn’t putting the files there.
CMP0126 relates to how the set(CACHE) command works. I don’t see that construct anywhere in this directory hierarchy so it seems to be completely superfluous.
CMP0157 is exercised here. I can understand that, but shouldn’t the cmake_minimum_required invocation in this file just specify 3.29, making all property setting needless? Do CMake’s current tests get used on older versions of CMake?
Pretty much all the tests require that. Xcode’s handling for shared libraries isn’t perfect, and Windows until 5.8/5.9 didn’t support static libraries.
I don’t remember offhand why the policy settings were required, perhaps Evan does?
