Skip to content

[cmake] Fix a few issues around swift cmake support build/test #37838

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 9, 2021
Merged

[cmake] Fix a few issues around swift cmake support build/test #37838

merged 2 commits into from
Jun 9, 2021

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Jun 8, 2021

Specifically I did the following:

  1. add_swift_host_tool was adding a -L flag for the just built runtime to host code (before the -L to the host runtime!). This is incorrect and caused us to have a race that sometimes resulted in us mixing/matching binary artifacts and other weirdness.
  2. I changed add_swift_host_library and add_swift_host_tool to not assume that the subdirectory we are in defines LLVM_COMMON_DEPENDS and to instead just respect it if it is set. The reason I did this is philosophically I don't think that things that do not depend on LLVM libraries (like my swift-cmake tests which are really simple) should have to define LLVM_COMMON_DEPENDS and also it makes it so that swift-cmake is no longer dependent on specific tools being built (like swift syntax) which causes these tests to not be built if the tools are not built. This test is meant to make sure that swift host side toolchain can build simple swift libraries and smoke out any errors, not actually depend on the tools we build.

gottesmm added 2 commits June 8, 2021 14:42
@gottesmm
[cmake] Do not use the just built swift link directory for host side …
db370de 
…executables.

This is just cruft that remained from the time when swift's host/target used the
same cmake code. Host tools do not statically link against just built artifacts.
@gottesmm
[cmake] Rather than forcing parts of the build that don't depend on L…
98ab14d 
…LVM to define LLVM_COMMON_DEPENDS, just have add_swift_host_library respect the global if it is set.
@gottesmm gottesmm requested review from compnerd and ahoppen June 8, 2021 21:46
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 8, 2021

@swift-ci smoke test

@gottesmm gottesmm requested a review from drexin June 8, 2021 21:46
@gottesmm gottesmm mentioned this pull request Jun 8, 2021
Closed
drexin
drexin approved these changes Jun 9, 2021
View reviewed changes
Copy link
Contributor

@drexin drexin left a comment

Choose a reason for hiding this comment

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

LGTM

@drexin
Copy link
Contributor

drexin commented Jun 9, 2021

@swift-ci smoke test windows

@gottesmm gottesmm merged commit 67176e7 into swiftlang:main Jun 9, 2021
@gottesmm gottesmm deleted the pr-1f1e2f54e1cefb7ac93a086a33295d1ce75a4094 branch June 9, 2021 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drexin drexin drexin approved these changes

@compnerd compnerd Awaiting requested review from compnerd

@ahoppen ahoppen Awaiting requested review from ahoppen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gottesmm @drexin