SwiftRemoteMirror: add a host build for the tools #21904

compnerd · 2019-01-16T05:53:05Z

This adds an explicit version of the SwiftRemoteMirror library for use
in the tools that comprise the toolchain. This is a separate build from
the target specific builds of the library even though we may be building
the runtime for the (same) host.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

compnerd · 2019-01-16T05:53:13Z

@swift-ci please test

compnerd · 2019-01-16T05:53:38Z

@fredriss @gottesmm - I was thinking something like this

swift-ci · 2019-01-16T06:17:48Z

Build failed
Swift Test OS X Platform
Git Sha - 103bcb92b5bd3401286260b6368f1fe6a90c53a8

compnerd · 2019-01-16T07:34:50Z

@swift-ci please test macOS platform

swift-ci · 2019-01-16T08:21:54Z

Build failed
Swift Test OS X Platform
Git Sha - 103bcb92b5bd3401286260b6368f1fe6a90c53a8

compnerd · 2019-01-16T19:13:29Z

@swift-ci please test

swift-ci · 2019-01-16T19:15:34Z

Build failed
Swift Test Linux Platform
Git Sha - 103bcb92b5bd3401286260b6368f1fe6a90c53a8

swift-ci · 2019-01-16T19:18:23Z

Build failed
Swift Test OS X Platform
Git Sha - 103bcb92b5bd3401286260b6368f1fe6a90c53a8

fredriss · 2019-01-16T22:23:10Z

When I try your patch, I get a different issue as the one in the PR testing:
CMake Error at stdlib/public/SwiftRemoteMirror/CMakeLists.txt:21 (add_custom_target): add_custom_target cannot create target "swiftRemoteMirror-macosx" because another target with the same name already exists. The existing target is a custom target created in source directory "/Users/friss/dev/github/stable/swift/stdlib/public/SwiftRemoteMirror". See documentation for policy CMP0002 for more details.

My build-script invocation:
./swift/utils/build-script --release-debuginfo --no-assertions --verbose-build=1 --build-ninja --skip-build-benchmarks --build-swift-examples=0 --build-swift-dynamic-stdlib=0 --build-swift-static-stdlib=0 --build-swift-dynamic-sdk-overlay=0 --build-swift-static-sdk-overlay=0 --reconfigure --build-toolchain-only=1 --swift-include-tests=0 --llvm-include-tests=0 --lldb

fredriss · 2019-01-16T22:28:45Z

I think I understand. add_swift_target_library above is called, because SWIFT_BUILD_REMOTE_MIRROR is defined. And I suppose this will create a swiftRemoteMirror-macos target

compnerd · 2019-01-16T23:19:13Z

Ugh, you're right. My condition isn't strict enough for avoiding the second definition.

fredriss · 2019-01-17T00:46:14Z

I managed to build locally with a variant of your patch, where I removed the SWIFT_BUILD_REMOTE_MIRROR condition from the first if and moved it to the one you added. I don't know under which circumstances SWIFT_BUILD_REMOTE_MIRROR is defined, so this might be totally wrong, but ti works for my config.

compnerd · 2019-01-17T03:46:48Z

SWIFT_BUILD_REMOTE_MIRROR is a user visible option, just need to ensure that I have it replicated on both sides. I'm very confused by the mac failure here though. Awesome, so sounds like getting that fixed up is the thing to do. Hopefully I can figure this out to get you unblocked.

compnerd · 2019-01-17T03:48:30Z

@swift-ci please test

swift-ci · 2019-01-17T03:50:26Z

Build failed
Swift Test Linux Platform
Git Sha - 700cf58ca62959c07f2b63965101fb1e06c91293

swift-ci · 2019-01-17T03:50:38Z

Build failed
Swift Test OS X Platform
Git Sha - 700cf58ca62959c07f2b63965101fb1e06c91293

compnerd · 2019-01-19T06:30:02Z

@swift-ci please test macOS platform

swift-ci · 2019-01-19T07:16:18Z

Build failed
Swift Test OS X Platform
Git Sha - e691ed5f944ca85d9fca396db9923ec63459b321

compnerd · 2019-01-19T08:12:48Z

@swift-ci please test

swift-ci · 2019-01-19T08:38:46Z

Build failed
Swift Test OS X Platform
Git Sha - d013c257fd7137cce64f36d4b005bc0df02cbce2

compnerd · 2019-01-19T18:18:52Z

Hmm, I think that the fat linking might be getting confused as the swift-reflection-test is attempting to link the host library rather than the target library.

compnerd · 2019-01-20T06:44:09Z

@swift-ci please test

fredriss · 2019-01-23T18:21:04Z

@swift-ci test

compnerd · 2019-01-24T00:45:24Z

@fredriss - this is basically just waiting on an okay from @gottesmm or you ... I think that this is good to go though

gottesmm · 2019-01-24T05:43:57Z

If Fred is cool with this, I am cool with this.

fredriss · 2019-01-25T01:33:59Z

Sorry for the delay, took me some time to get back to this.

I just tried this and it still doesn't work for me in the way I already described above. SWIFT_BUILD_REMOTE_MIRROR and SWIFT_INCLUDE_TOOLS will be defined in my build configuration. This will create duplicate targets in stdlib/public/SwiftRemoteMirror/CMakeLists.txt.

I think we need to remove the SWIFT_BUILD_REMOTE_MIRROR condition from the first condition in this CMakeList.txt and add it to the conditional block this PR is adding. This seems to get the build further for me.

fredriss · 2019-01-25T01:39:54Z

stdlib/public/SwiftRemoteMirror/CMakeLists.txt

@@ -2,14 +2,35 @@
 # always built as a shared library.
 if(SWIFT_BUILD_DYNAMIC_STDLIB OR SWIFT_BUILD_REMOTE_MIRROR)


I think SWIFT_BUILD_REMOTE_MIRROR needs to be removed from here and moved to the new host code. Not sure what this build setting is for. Is it always set at the same time as SWIFT_INCLUDE_TOOLS, or is it different?)

It is up to the user to decide if the remote mirror should be built or not. Personally, I am of the opinion that if Apple doesn't build in a configuration where it matters, we should remove the option and always build it when SWIFT_INCLUDE_TOOLS is defined. We now have a differentiation between the host and target bits, and if we are building tools, we should just build it for the host.

It's unclear to me whether you're agreeing with me that the SWIFT_BUILD_REMOTE_MIRROR should be moved to the host part, or whether you're arguing something different

I'm arguing for something more drastic (the removal of the option). I'm interested in how the current version doesn't work though as the if(NOT SWIFT_BUILD_STDLIB AND NOT SWIFT_BUILD_REMOTE_MIRROR) should prevent the duplicate umbrella target. Note that if we can understand why this condition is insufficient, then I'm also okay with getting that changed and merged to get you unblocked.

The issue is not a duplicate target in this case, but missing targets. add_swift_target_library will create build targets for all the different targets (iphonesimulator, watchsimulator, ...) that have been configured to build. But the swiftReflection side never ran add_swift_target_library(), so the reflection dependencies of those build targets don't exist. Sorry for the overloaded use of 'target'. Here's an example:

CMake Error at cmake/modules/AddSwift.cmake:2277 (add_dependencies):
The dependency target "swiftReflection-iphonesimulator" of target
"swiftRemoteMirror-iphonesimulator-i386" does not exist.
Call Stack (most recent call first):
stdlib/public/SwiftRemoteMirror/CMakeLists.txt:18 (add_swift_target_library)

fredriss

Alos, I think we have one build configuration where we build only RemoteMirrors for some external tools to use. This might be the reason for the existence of SWIFT_BUILD_REMOTE_MIRROR.

fredriss · 2019-01-28T23:25:29Z

stdlib/public/SwiftRemoteMirror/CMakeLists.txt

@@ -2,14 +2,35 @@
 # always built as a shared library.
 if(SWIFT_BUILD_DYNAMIC_STDLIB OR SWIFT_BUILD_REMOTE_MIRROR)


The issue is not a duplicate target in this case, but missing targets. add_swift_target_library will create build targets for all the different targets (iphonesimulator, watchsimulator, ...) that have been configured to build. But the swiftReflection side never ran add_swift_target_library(), so the reflection dependencies of those build targets don't exist. Sorry for the overloaded use of 'target'. Here's an example:

CMake Error at cmake/modules/AddSwift.cmake:2277 (add_dependencies):
The dependency target "swiftReflection-iphonesimulator" of target
"swiftRemoteMirror-iphonesimulator-i386" does not exist.
Call Stack (most recent call first):
stdlib/public/SwiftRemoteMirror/CMakeLists.txt:18 (add_swift_target_library)

compnerd · 2019-01-29T00:25:54Z

@fredriss hmm, so it should be okay to remove SWIFT_BUILD_REMOTE_MIRROR in the target case and SWIFT_BUILD_DYNAMIC_STDLIB in the host case right?

fredriss · 2019-01-29T01:06:08Z

I think this would work.

compnerd · 2019-01-29T01:15:09Z

Sweet! I'm cool with that then! Lets get that taken care of :-)

compnerd · 2019-01-29T01:15:13Z

@swift-ci please test

swift-ci · 2019-01-29T01:17:20Z

Build failed
Swift Test Linux Platform
Git Sha - 5bd9655687a4f1b8e30fb8d2e9d4be6162468530

This adds an explicit version of the SwiftRemoteMirror library for use in the tools that comprise the toolchain. This is a separate build from the target specific builds of the library even though we may be building the runtime for the (same) host.

swift-ci · 2019-01-29T02:01:30Z

Build failed
Swift Test OS X Platform
Git Sha - 5bd9655687a4f1b8e30fb8d2e9d4be6162468530

compnerd · 2019-01-29T02:01:37Z

@swift-ci please test

swift-ci · 2019-01-29T02:03:42Z

Build failed
Swift Test Linux Platform
Git Sha - 7aa4948a6cee55f36ee6e56a069bed9fe6c20aa5

swift-ci · 2019-01-29T04:13:20Z

Build failed
Swift Test OS X Platform
Git Sha - 7aa4948a6cee55f36ee6e56a069bed9fe6c20aa5

fredriss · 2019-01-29T17:09:48Z

This seems to work for LLDB. @Rostepher will this have any impact on the SwiftMirrors standalone build?

Rostepher · 2019-01-30T22:57:19Z

We do use the SWIFT_BUILD_REMOTE_MIRROR variable when running the remote_mirror_* presets, which are in-use. I think un-gating the dynamic library build will break things for us.

zisko · 2019-01-30T23:03:35Z

Spoke with @Rostepher. We should merge this for LLDB, and find a different solution for remote mirror only builds.

compnerd · 2019-01-30T23:33:46Z

@fredriss @zisko @Rostepher - this is ready, just waiting for you guys to say when to merge this change

fredriss · 2019-02-04T23:04:07Z

Sorry, I should have merged this a while ago.

compnerd mentioned this pull request Jan 16, 2019

Fix LLDB build when not building the standard library. #21796

Closed

compnerd force-pushed the mirror-mirror-on-the-wall branch from 103bcb9 to 700cf58 Compare January 16, 2019 19:12

compnerd force-pushed the mirror-mirror-on-the-wall branch from 700cf58 to e691ed5 Compare January 17, 2019 03:48

compnerd force-pushed the mirror-mirror-on-the-wall branch from e691ed5 to d013c25 Compare January 19, 2019 08:12

compnerd force-pushed the mirror-mirror-on-the-wall branch from d013c25 to 5bd9655 Compare January 20, 2019 06:43

fredriss reviewed Jan 25, 2019

View reviewed changes

fredriss reviewed Jan 28, 2019

View reviewed changes

compnerd force-pushed the mirror-mirror-on-the-wall branch from 5bd9655 to 7aa4948 Compare January 29, 2019 01:15

compnerd force-pushed the mirror-mirror-on-the-wall branch from 7aa4948 to 85f45df Compare January 29, 2019 02:01

fredriss merged commit b607717 into swiftlang:master Feb 4, 2019

compnerd deleted the mirror-mirror-on-the-wall branch February 5, 2019 02:48

		@@ -2,14 +2,35 @@
		# always built as a shared library.
		if(SWIFT_BUILD_DYNAMIC_STDLIB OR SWIFT_BUILD_REMOTE_MIRROR)

SwiftRemoteMirror: add a host build for the tools #21904

SwiftRemoteMirror: add a host build for the tools #21904

Uh oh!

Conversation

compnerd commented Jan 16, 2019

Uh oh!

compnerd commented Jan 16, 2019

Uh oh!

compnerd commented Jan 16, 2019

Uh oh!

swift-ci commented Jan 16, 2019

Uh oh!

compnerd commented Jan 16, 2019

Uh oh!

swift-ci commented Jan 16, 2019

Uh oh!

compnerd commented Jan 16, 2019

Uh oh!

swift-ci commented Jan 16, 2019

Uh oh!

swift-ci commented Jan 16, 2019

Uh oh!

fredriss commented Jan 16, 2019

Uh oh!

fredriss commented Jan 16, 2019

Uh oh!

compnerd commented Jan 16, 2019

Uh oh!

fredriss commented Jan 17, 2019

Uh oh!

compnerd commented Jan 17, 2019

Uh oh!

compnerd commented Jan 17, 2019

Uh oh!

swift-ci commented Jan 17, 2019

Uh oh!

swift-ci commented Jan 17, 2019

Uh oh!

compnerd commented Jan 19, 2019

Uh oh!

swift-ci commented Jan 19, 2019

Uh oh!

compnerd commented Jan 19, 2019

Uh oh!

swift-ci commented Jan 19, 2019

Uh oh!

compnerd commented Jan 19, 2019

Uh oh!

compnerd commented Jan 20, 2019

Uh oh!

fredriss commented Jan 23, 2019

Uh oh!

compnerd commented Jan 24, 2019

Uh oh!

gottesmm commented Jan 24, 2019

Uh oh!

fredriss commented Jan 25, 2019

Uh oh!

fredriss Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

fredriss Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredriss Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

fredriss left a comment

Choose a reason for hiding this comment

Uh oh!

fredriss Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 26, 2019 •

edited

Loading