Skip to content

Runtimes: add an initial build of Observation #83006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

compnerd
Copy link
Member

This migrates Observation to the new runtimes build. It requires some additional help during the build to locate the internal CMakeConfig.h header from the runtime build. However, this is a good skeleton for us to start addressing some of those issues.

This migrates Observation to the new runtimes build. It requires some
additional help during the build to locate the internal `CMakeConfig.h`
header from the runtime build. However, this is a good skeleton for us
to start addressing some of those issues.
@compnerd
Copy link
Member Author

@swift-ci please smoke test

Copy link
Contributor

@edymtt edymtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reasonable starting point -- left a few comments to note some things that stood out to me, but nothing blocking a merge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we want to copy this like we are doing for Core and Synchronization in Resync.cmake -- unless I'm missing the rational to change approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that copying the file is the right thing to do - if we cannot isolate it away, I think that we should have the orchestration wire up the internals of the previous build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if @etcwilde has opinions here


# Configure plist creation for Darwin platforms.
generate_plist("${CMAKE_PROJECT_NAME}" "${CMAKE_PROJECT_VERSION}" swiftObservation)
embed_manifest(swiftObservation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be done at a later time: we should remember to add another affordance for vendor overrides, e.g.

Suggested change
embed_manifest(swiftObservation)
embed_manifest(swiftObservation)
include("${${PROJECT_NAME}_VENDOR_MODULE_DIR}/swiftObservation.cmake" OPTIONAL)

Comment on lines +85 to +86
set_target_properties(swiftObservation PROPERTIES
Swift_MODULE_NAME Observation)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional at this stage: we should specify the linker language since we have a C++ source file

Suggested change
set_target_properties(swiftObservation PROPERTIES
Swift_MODULE_NAME Observation)
set_target_properties(swiftObservation PROPERTIES
Swift_MODULE_NAME Observation
LINKER_LANGUAGE CXX)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to link with the C++ driver, we want to link with the Swift driver. We could be explicit and use the LINKER_LANGUAGE Swift, but I don't think that adds anything of value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed we needed to link with the C++ driver to ensure we link the correct C++ standard library, as per discussion in #79972 and #80139 -- I'm fine with not forcing it for now and see if we hit issues later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selection of the c++ standard library should be driven by a flag in the swift-driver - we need this functionality for the C++ interop anyway. I believe that the current best option is -Xcc -stdlib=... -Xlinker -Xcc -Xlinker -stdlib=... (CC: @artemcm)

Copy link
Contributor

@justice-adams-apple justice-adams-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending very small typo and the outstanding question of copying (or not) the plist.in file

VERSION 6.1.0${BUILD_NUMBER})

if(NOT PROJECT_IS_TOP_LEVEL)
message(SEND_ERROR "Swift Synchronization must build as a standalone project")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
message(SEND_ERROR "Swift Synchronization must build as a standalone project")
message(SEND_ERROR "Swift Observation must build as a standalone project")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof ... thanks! Is there a previous thread about the plist.in copying? The thread on this PR is related to CMakeConfig.h generated in the Swift build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants