Generate overlay dependencies with utils/find-overlay-dependencies.sh
#5017
Conversation
|
@swift-ci Please smoke test |
| @@ -3,6 +3,8 @@ add_swift_library(swiftAVFoundation ${SWIFT_SDK_OVERLAY_LIBRARY_BUILD_TYPES} IS_ | |||
| TARGET_SDKS OSX IOS IOS_SIMULATOR TVOS TVOS_SIMULATOR | |||
| SWIFT_COMPILE_FLAGS "${SWIFT_RUNTIME_SWIFT_COMPILE_FLAGS}" | |||
| LINK_FLAGS "${SWIFT_RUNTIME_SWIFT_LINK_FLAGS}" | |||
| SWIFT_MODULE_DEPENDS Foundation CoreMedia QuartzCore | |||
| SWIFT_MODULE_DEPENDS_OSX AppKit | |||
| SWIFT_MODULE_DEPENDS Foundation CoreMedia | |||
There was a problem hiding this comment.
Should we drop the non-OS-specific lines now that it's explicit for every OS?
There was a problem hiding this comment.
I could go either way on it.
There was a problem hiding this comment.
Actually, this is used all over in stdlib/private and I'm only changing the public directory for now.
There was a problem hiding this comment.
I feel like the lines should either be autogenerated (and have a comment saying such) or should contain manual dependencies. Doing both is odd.
| @@ -8,6 +8,6 @@ add_swift_library(swiftCoreGraphics ${SWIFT_SDK_OVERLAY_LIBRARY_BUILD_TYPES} IS_ | |||
| SWIFT_COMPILE_FLAGS "${SWIFT_RUNTIME_SWIFT_COMPILE_FLAGS}" | |||
| LINK_FLAGS "${SWIFT_RUNTIME_SWIFT_LINK_FLAGS}" | |||
| SWIFT_MODULE_DEPENDS ObjectiveC Dispatch Darwin | |||
| SWIFT_MODULE_DEPENDS_OSX IOKit XPC | |||
| SWIFT_MODULE_DEPENDS_OSX Dispatch IOKit ObjectiveC os | |||
There was a problem hiding this comment.
This one should probably have _IOS, _TVOS, _WATCHOS entries too. Maybe the script should complain if there's no entry for a particular SDK.
There was a problem hiding this comment.
I am going off of the TARGET_SDKS OSX IOS IOS_SIMULATOR TVOS TVOS_SIMULATOR lines in the CMakeLists.txt files for each overlay. CoreGraphics doesn't have a TARGET_SDKS line so I'm reluctant to add more platforms. I'm just refactoring--someone who knows which platforms each supports should do it. (Or I can, if I learn how to tell which overlays support which platforms).
There was a problem hiding this comment.
A lack of TARGET_SDKS means all four Apple platforms (plus simulators).
| SCRIPT="$(dirname "$0")/find-overlay-dependencies.sh" | ||
|
|
||
| # Don't update XCTest | ||
| for overlay in $(find ./stdlib/public/SDK/ -depth 1 -type d ! -name XCTest -exec basename \{\} \;); do |
There was a problem hiding this comment.
Should this be $0-relative? (Alternately, should it complain if you're not in a directory where ./stdlib/public/SDK/ exists?)
| if [[ $# -ne 1 ]]; then | ||
| echo 'usage:' $0 '<module-name>' >&2 | ||
| function usage() { | ||
| echo 'usage:' $0 '<module-name> [update]' >&2 |
There was a problem hiding this comment.
Any reason not to squash the loop script into this one? (invoking itself with $0). It seems tidier for utils/.
| UPDATE_CMAKE=0 | ||
| case $# in | ||
| 1) ;; | ||
| 2) if [[ $2 != 'update' ]]; then |
There was a problem hiding this comment.
Nitpick: why an extra postfix command rather than a flag? (-update)
There was a problem hiding this comment.
I didn't want to use getopts. No reason really.
| UPDATE_CMAKE=1 ;; | ||
| *) | ||
| usage ;; | ||
| esac | ||
|
|
||
| OVERLAYS_PATH=$(dirname "$0")/../stdlib/public/SDK/ |
There was a problem hiding this comment.
This should probably be consistent with the -loop script in using $PWD or not.
| ALL_OVERLAYS=() | ||
| for overlay in $(ls "$OVERLAYS_PATH"); do | ||
| for overlay in $(find $OVERLAYS_PATH -depth 1 -type d ! -name XCTest -exec basename \{\} \;); do |
There was a problem hiding this comment.
The quotes are probably still important—did you test with spaces in the path?
| echo # newline | ||
| echo " $DEPENDS_ON" | ||
| if [[ $UPDATE_CMAKE == 1 ]]; then | ||
| perl -pi -e "s/^ $CMAKE_DEPENDS_NAME[$sdk].*$/ $CMAKE_DEPENDS_NAME[$sdk] $DEPENDS_ON/" $CMAKE_PATH |
There was a problem hiding this comment.
Nitpick: can you use sed instead of perl? One fewer dependency, even if Perl is installed on most systems by default now.
There was a problem hiding this comment.
I was using sed, but there's not a portable way to do it for OSX and linux.
http://stackoverflow.com/questions/5694228/sed-in-place-flag-that-works-both-on-mac-bsd-and-linux
sed -i -e "s/^ $CMAKE_DEPENDS_NAME[$sdk].*$/ $CMAKE_DEPENDS_NAME[$sdk] $DEPENDS_ON/" $CMAKE_PATH
Basically it works but creates extra files like CMakeLists.txt-e for every file it updates.
There was a problem hiding this comment.
We don't need to be portable to Linux for this; only Darwin platforms have a modularized SDK anyway.
|
@erg, @shahmishal, and I talked about some possible testing strategies here; the one I think we were leaning towards was making a mirror of the stdlib/ directory structure and CMakeLists files and running the script on that instead of the one in SRCROOT, then doing a diff. That can come in a follow-up commit, though. |
|
@jrose-apple @erg @shahmishal seding the cmake lists files seems like a really brittle and hacky solution. I am not sure if there is a better solution if we want this information at the add_library site. I almost wonder though since we are suggesting a model where the dependency information is generated for us, why can't we just teach cmake how to generate this information internally. Then we do not need to do anything. That might be too magical perhaps? |
|
Specifically, I think we would create a clang tool that would take in a bunch of modules/sdks and then output the data in a json format and perhaps work in parallel. As an additional note, I think this code is complicated enough that we could benefit from moving this into python or at least doing the work in parallel using xargs. |
|
Actually now that I think about it, the list of module dependencies is so long, we really do not get any value from listing them in the cmakelists file. An implicit cmake solution would mean that we would not even have to run a script. It is just a question of can we make this fast enough to complete quickly. My feeling is that the main issue here is that on macOS process startup can be slow so that is why it would be good to have just one clang process that does this. Doesn't libclang support stuff like this? |
|
I'm all for automating it eventually, but this is an easy way to get the dependencies fixed right now manually. At least we won't get build failures for a week after changing the overlays (like we did this time). |
…erlays are added the build breaks. There's already a tool to get proper dependencies, `utils/find-overlay-dependencies.sh`, so this patch allows that tool to update the `CMakeLists.txt` files in-place. Also it adds a line to the `CMakeLists.txt` files for each SDK so that the tool works.
erg
force-pushed
the
overlay-dependencies
branch
from
848f86b to
6c6f30b
Compare
|
@swift-ci please smoke test |
|
Again, this script is more expensive than I want to run on every configuration, and most of that is inherent in Clang module building (i.e. it's not going to get faster). I'd be happy to take a patch for importing the data from some structured format, but it seems like that shouldn't block this change ("perfect is the enemy of good" and all). |
|
It's certainly possible to parallelize this too. |
|
@erg @jrose-apple I agree with you that this is a stop gap solution. But at the same time I do not buy that this actually takes 2 minutes like you said in that email. Nothing here is multithreaded unless I am missing something yet it seems perfectly parallel. Thus 120 seconds / 8 => 15 seconds. I am sure if we got rid of process time come up it could be even faster. |
|
I would gestimate 5-10 seconds. |
|
Metacomment. The LTO build I think is broken b/c of this so I am blocked from landing some LTO build fixes. So if there are any further refactorings, I would ask that you land the code now. |
|
@swift-ci Please smoke test linux platform |
erg
force-pushed
the
overlay-dependencies
branch
from
6c6f30b to
140739e
Compare
|
@swift-ci please smoke test linux platform |
|
@swift-ci please smoke test |
|
@gottesmm If the new smoke tests pass, I think it's fine to merge for now. I will make another pass to clean something up as I've learned some more zsh. But this should be way better than it is right now. |
erg
force-pushed
the
overlay-dependencies
branch
from
140739e to
6c6f30b
Compare
|
Latest patch caused circularity. I reverted to a version that passed the builds. It's testing again. |
|
@swift-ci please smoke test linux platform |
1 similar comment
|
@swift-ci please smoke test linux platform |
|
@swift-ci Please smoke test Linux |
The overlay dependencies right now are approximate, and when new overlays are
added the build breaks. There's already a tool to get proper
dependencies,
utils/find-overlay-dependencies.sh, so this patchallows that tool to update the
CMakeLists.txtfiles in-place.Also it adds a line to the
CMakeLists.txtfiles for each SDK so that the tool works.