[CMake] Fix dependency on the build directory

[davezarzycki]

SWIFT_NATIVE_SWIFT_TOOLS_PATH can be set to a path outside of the build directory whereas SWIFT_RUNTIME_OUTPUT_INTDIR always points to the build directory.

[davezarzycki]

@swift-ci please smoke test

[compnerd]

I'm not sure if anyone is relying on the ability to run the test suite with prebuilt binaries (I could see that being useful for running the standard library tests).

If we are removing the use of the prebuilt path, assuming that no one is relying on that quirk, I am not sure I understand why this is better than say replacing L117-L125 with L120. That actually will have CMake generate the proper dependency and we don't need to worry about the path computation or string operations and is much simpler.

CC: @drexin

[davezarzycki]

@compnerd – Unless I'm missing something, this only changes how build target dependencies are calculated. Without this change, the testing targets fail due to a missing build target.

[compnerd]

Correct, it changes how the build target dependencies are calculated. But, why calculate the dependencies manually when CMake can do it for us? I'm saying that we can just take the path that is currently marked as the Xcode path and get the same behaviour in a concise and consistent manner. Does that not accomplish the same thing that your change does?

The second point is related to the fact that the lit effectively uses which to find the tools, and previously if you set SWIFT_NATIVE_TOOLS_PATH to an external build along with the path, you might be able to get away with using a separate build to run the test suite. Or does something else break in that path and that usage could not have possibly worked? Is that not something we need to worry about?

[davezarzycki]

I switched to the Xcode approach and I verified that lit still uses the tools found from the output directory. Does this work for you @compnerd?

@swift-ci please smoke test

[compnerd]

I think that the comment is outdated, but, yes, that was exactly what I meant. The code change itself looks good to me.

Might be a good idea to double check with others that no one is relying on the custom PATH + SWIFT_NATIVE_TOOLS_PATH.

[davezarzycki]

Others? Who do you have in mind? Can CC them or should I post in the forums?

[compnerd]

I think that @atrick or @gottesmm might be a good bet for ideas of if anyone would be dependent on that.

[atrick]

I'm not personally using SWIFT_NATIVE_SWIFT_TOOLS_PATH, but I think it's an important feature for anyone working on the stdlib. Does this PR make it impossible to run the tests from a standalone stdlib build?

[davezarzycki]

This patch only changes CMake's target dependency generation.

[compnerd]

Thinking more about this ... the changes here should only impact the build times, not the workflow itself. We should keep the dependencies correct and we can always find a way to alleviate that if it is a true problem. Id say ping @gottesmm, but otherwise, I think that it is probably better to merge sooner rather than later.

[davezarzycki]

Fixed the comment. I'd like to point out that this does not "only impact the build times". If one sets SWIFT_NATIVE_SWIFT_TOOLS_PATH then the build will fail because a bogus dependency on a non-existent build target will be created and ninja check-swift etc will fail.

@swift-ci please smoke test

[davezarzycki]

@gottesmm? Did you see @compnerd's ping? I'd like to merge this. Thanks

[finagolfin]

I haven't looked into exactly how this affects using a prebuilt Swift compiler to build a standalone stdlib and test it- which "sets SWIFT_NATIVE_SWIFT_TOOLS_PATH"- but I'll note that is a convenient usecase that should be maintained. @gottesmm even added a build preset for it, 5f15385, that you could test with this pull to make sure it still works (you'll need my #32860 applied also).

[davezarzycki]

@buttaface – As I wrote earlier in this PR, this change only affects how build system dependencies are generated.

If I might repeat myself with different words: the current behavior is bogus. If one sets SWIFT_NATIVE_SWIFT_TOOLS_PATH to a path outside of the build directory, then ninja check-swift will fail to run due to a missing build target. That's not what anybody wants. Fixing that bug doesn't break the SWIFT_NATIVE_SWIFT_TOOLS_PATH usage model.

[atrick]

@davezarzycki the reason people are asking is that the diff appears to remove a special case that was deliberately added to support the native tools use case. So we'll just need to take your word that it still(?) works, or maybe continues to not work.. I can't tell

[finagolfin]

I just tested this pull by building and testing the standalone stdlib and that still works, as that preset disables SWIFT_INCLUDE_TOOLS and does a standalone build, so there are no deps_binaries to add. I have no idea if it's possible for someone to e.g. build sourcekit alone using an external prebuilt toolchain and then test it with a prebuilt sourcekitd-test, or whatever other scenario this special case was meant to support, so I'm inclined to approve this.

@davezarzycki, any idea what scenario this was added to support originally?

[davezarzycki]

Sorry for the earlier deleted comment. I'm not in front of my computer.

If I had to wager, then I'd bet that somebody just searched for SWIFT_RUNTIME_OUTPUT_INTDIR and replaced with SWIFT_NATIVE_SWIFT_TOOLS_PATH without thinking deeply about each replaced variable use and whether it worked by accident or by design.

[davezarzycki]

Now that I'm in front of my computer, I dug through the source history and unfortunately the history wasn't preserved across Swift being open sourced. Maybe somebody at Apple can dig further?

[atrick]

@davezarzycki, @buttaface already confirmed that the standalone preset+tests still works. I think that's all that matters aside from the change generally making sense, which it does.

[finagolfin]

Yep.

[davezarzycki]

Thank you everybody for the feedback