Add libQuote to apple/swift:tensorflow CI #26708
Conversation
|
As @rxwei and @dan-zheng explained to me, we currently don't run tests for swift-apis in apple/swift:tensorflow CI because that requires building the full toolchain which is quote slower that a regular build. Since I added libQuote to the build in exactly the same way that swift-apis has been added, we also don't run tests for libQuote here. As a result, changes in QuoteTransform.cpp may very well break libQuote tests, and this PR doesn't help preventing that. This is somewhat unsatisfying but definitely better than nothing (since we at least get to make sure that libQuote builds with our toolchain). |
|
@swift-ci please clean test tensorflow |
| # Copy Quote sources, if they exist. | ||
| if (TENSORFLOW_SWIFT_QUOTE) | ||
| file(GLOB_RECURSE TENSORFLOW_SWIFT_QUOTE_SOURCES | ||
| "${TENSORFLOW_SWIFT_QUOTE}/Sources/Quote/*.swift") |
There was a problem hiding this comment.
This is really an anti-pattern for CMake. If you do this, subsequent incremental builds will be incorrect as this will not pick up new files being added - it is evaluated once. It is much better to explicitly list out the sources.
There was a problem hiding this comment.
What's the right way of doing this? My knowledge of CMake is cursory at best, so I'd appreciate any help :)
There was a problem hiding this comment.
It is to explicitly list out all the source files manually. Doing so allows CMake to indicate to the build tool which files to track. You would literally just enumerate the files and write them out.
There was a problem hiding this comment.
Just a note: the file list here actually refers to files from a different repository (a different build-script product).
In practice, this means that when files are added/removed, we'll need to update the CMake source list when we update utils/update_checkout/update_checkout_config.json.
|
@swift-ci please clean test tensorflow |
| endif() | ||
|
|
||
| add_swift_target_library(swiftQuote ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} IS_STDLIB | ||
| "${SOURCES}" |
There was a problem hiding this comment.
BTW, once you actually list out the files, you can just delete the SOURCES variable and inline the full list of files.
|
Given the ongoing discussion about moving the TensorFlow module out of apple/swift:tensorflow, I think this PR is no longer relevant. Once the new infrastructure for the TensorFlow module is rolled out, we should do the same for Quote. Until then, au revoir. @compnerd Thank you for your valuable feedback on CMake! I learned a lot from it. |
Now that libQuote has been opensourced at https://github.com/tensorflow/swift, it would be good to get it into CI to make sure we don't break quoting as we keep on merging with the recent master.