Skip to content
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

[CMake] Give a dedicated component to compiler swift-syntax libraries #76497

Merged
merged 3 commits into from
Sep 17, 2024
Merged

[CMake] Give a dedicated component to compiler swift-syntax libraries #76497

merged 3 commits into from
Sep 17, 2024

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Sep 16, 2024 •
edited
Loading

'compiler-swift-syntax-lib' so projects statically link to compiler libraries (libswiftAST etc) can use the required shared libraries.

bnbarham
bnbarham approved these changes Sep 16, 2024
View reviewed changes
cmake/modules/SwiftComponents.cmake
@@ -97,10 +98,10 @@ macro(swift_configure_components)
set(SWIFT_INSTALL_COMPONENTS "${_SWIFT_DEFAULT_COMPONENTS}" CACHE STRING
"A semicolon-separated list of components to install from the set ${_SWIFT_DEFINED_COMPONENTS}")

# 'compiler' depends on 'swift-syntax-lib' component.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess theoretically this could have been removed when we did the split 😅?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I totally forgot about this.

@rintaro
Copy link
Member Author

rintaro commented Sep 16, 2024

@swift-ci Please smoke test

@rintaro
[CMake] Give a dedicated component to compiler swift-syntax libraries
38be0d3 
'compiler-swift-syntax-lib' so projects statically link to compiler
libraries (libAST etc) can use the required shared libraries.

rdar://135923606
@rintaro rintaro force-pushed the compilerswiftsyntax-component-rdar135923606 branch from 998ce5b to 38be0d3 Compare September 16, 2024 21:47
@rintaro
Copy link
Member Author

rintaro commented Sep 16, 2024

@swift-ci Please smoke test

compnerd
compnerd requested changes Sep 16, 2024
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This needs to change the caches for Windows.

@compnerd
Copy link
Member

@swift-ci please build toolchain Windows platform

@bnbarham
Copy link
Contributor

This needs to change the caches for Windows.

Pretty sure we're fine there?

set(SWIFT_INSTALL_COMPONENTS
      autolink-driver
      compiler
      clang-builtin-headers
      editor-integration
      tools
      sourcekit-inproc
      static-mirror-lib
      swift-remote-mirror
      swift-remote-mirror-headers
      swift-syntax-lib
    CACHE STRING "")

compiler includes compiler-swift-syntax and swift-syntax-lib is specified explicitly

@rintaro
Copy link
Member Author

rintaro commented Sep 16, 2024

This needs to change the caches for Windows.

So every cmake/caches/*.cmake that has compiler in SWIFT_INSTALL_COMPONENTS requires compiler-swift-syntax-lib too?

@compnerd
Copy link
Member

@bnbarham oh, ewww ... you want to rely on the component being implicitly added? That sounds pretty ... scary

@bnbarham
Copy link
Contributor

@bnbarham oh, ewww ... you want to rely on the component being implicitly added? That sounds pretty ... scary

🤷 I'm fine adding them explicitly. I'm just saying it should work as is.

@rintaro
Copy link
Member Author

rintaro commented Sep 16, 2024 •
edited
Loading

Since I found cmake/caches/Linux-x86_64.cmake doesn't include swift-syntax-lib, I updated them all to include compiler-swift-syntax-lib explicitly too.

@rintaro
Copy link
Member Author

rintaro commented Sep 16, 2024

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Sep 16, 2024

@swift-ci Please build toolchain Windows

@rintaro
Add back implicit swift-syntax-lib to compiler component for now
d6cf1e0 
Some clients doesn't specify `swift-syntax-lib`.
@rintaro
Copy link
Member Author

rintaro commented Sep 16, 2024

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Sep 16, 2024

@swift-ci Please build toolchain Windows

@rintaro rintaro merged commit e916e9e into swiftlang:main Sep 17, 2024
4 checks passed
@drodriguez drodriguez mentioned this pull request Sep 20, 2024
Merged
drodriguez added a commit that referenced this pull request Sep 20, 2024
@drodriguez
[cmake] Add missing dependency between component and its targets (#76596
d3b788f 
)

`swift_install_in_component` does not create a dependency between its
`TARGETS` and the `COMPONENT`, so one has to be created manually.
The missing dependency might cause invocations to CMake/Ninja for the
`install-*` targets to not build the required files before the
installation is performed. Because the normal `build-script` builds the
`all` target, this kind of missing dependencies like this are common.

This bit of code appeared in #76497 some days ago.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd approved these changes

@bnbarham bnbarham bnbarham approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@rintaro @compnerd @bnbarham