Skip to content

[CMake] Add option to set the target namespace #2659

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 1 commit into from
Jun 21, 2024
Merged

[CMake] Add option to set the target namespace #2659

merged 1 commit into from
Jun 21, 2024

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented May 17, 2024

Each target name is prefixed with SWIFTSYNTAX_TARGET_NAMESPACE

Also, add SWIFTSYNTAX_EMIT_MODULE setting; true for building library-evolution enabled modules with .swiftinterface, false for building fragile modules, undefined is true

@rintaro rintaro requested review from ahoppen and bnbarham as code owners May 17, 2024 22:34
@rintaro rintaro force-pushed the cmake-targetprefix branch from da1afec to 93fc0a1 Compare May 17, 2024 22:35
@rintaro
Copy link
Member Author

rintaro commented May 17, 2024

@swift-ci Please test

@rintaro rintaro force-pushed the cmake-targetprefix branch from 93fc0a1 to 97805e9 Compare May 20, 2024 21:11
@rintaro
Copy link
Member Author

rintaro commented May 20, 2024

@swift-ci Please test

@rintaro rintaro force-pushed the cmake-targetprefix branch from 97805e9 to 2e419fd Compare May 21, 2024 00:14
@rintaro
Copy link
Member Author

rintaro commented May 21, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented May 21, 2024

@swift-ci Please test Windows

@rintaro
Copy link
Member Author

rintaro commented May 21, 2024

@swift-ci Please test

1 similar comment
@rintaro
Copy link
Member Author

rintaro commented May 21, 2024

@swift-ci Please test

@etcwilde
Copy link
Contributor

swiftlang/swift#73798

@swift-ci please test Linux

@etcwilde
Copy link
Contributor

@swift-ci please test Linux platform

@rintaro rintaro requested review from hamishknight May 24, 2024 16:49
@rintaro rintaro mentioned this pull request May 24, 2024
Merged
This was referenced Jun 7, 2024
Merged
Merged
@rintaro rintaro force-pushed the cmake-targetprefix branch from 2e419fd to 0a8476a Compare June 15, 2024 07:14
@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test Windows

2 similar comments
@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test Windows

@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test Windows

@rintaro rintaro force-pushed the cmake-targetprefix branch from 0a8476a to c2b41d8 Compare June 15, 2024 07:54
@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test

1 similar comment
@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test

@rintaro rintaro force-pushed the cmake-targetprefix branch from c2b41d8 to ea53d4d Compare June 15, 2024 08:09
@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test Windows

@rintaro rintaro force-pushed the cmake-targetprefix branch from ea53d4d to 26ae5e9 Compare June 15, 2024 13:23
@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Jun 15, 2024

@swift-ci Please test Windows

hamishknight
hamishknight approved these changes Jun 18, 2024
View reviewed changes
cmake/modules/AddSwiftHostLibrary.cmake Outdated
@@ -17,10 +17,21 @@
#
# Remove once rdar://102202478 is fixed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably we now need to use this function regardless of whether rdar://102202478 is fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Moved the comment to the -dep.swift hack.

cmake/modules/AddSwiftHostLibrary.cmake
Comment on lines 20 to +18
cmake_parse_arguments(ARGS "PUBLIC;PRIVATE;INTERFACE" "" "" ${ARGN})
foreach(DEPENDENCY ${ARGS_UNPARSED_ARGUMENTS})
set(link_type)
if(ARGS_PUBLIC)
set(link_type PUBLIC)
elseif(ARGS_PRIVATE)
set(link_type PRIVATE)
elseif(ARGS_INTERFACE)
set(link_type INTERFACE)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be an error if you omit the link type?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://cmake.org/cmake/help/latest/command/target_link_libraries.html#libraries-for-both-a-target-and-its-dependents
Since it's optional for target_link_libraries, I'm leaving it as is for now

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, my main concern would be if a new link type was added that wasn't handled here, it may get silently dropped

@rintaro
[CMake] Add option to set the target namespace
4e7bb71 
Each target name is prefixed with SWIFTSYNTAX_TARGET_NAMESPACE

Also, add SWIFTSYNTAX_EMIT_MODULE setting; 'true' for building
library-evolution enabled modules with .swiftinterface, 'false' for
building fragile modules, undefined is 'true'
@rintaro rintaro force-pushed the cmake-targetprefix branch from 26ae5e9 to 4e7bb71 Compare June 18, 2024 15:44
@rintaro
Copy link
Member Author

rintaro commented Jun 18, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Jun 18, 2024

@swift-ci Please test Windows

@rintaro rintaro merged commit d2adc61 into swiftlang:main Jun 21, 2024
3 checks passed
@rintaro rintaro mentioned this pull request Jun 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen Awaiting requested review from ahoppen ahoppen is a code owner

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

@hamishknight hamishknight Awaiting requested review from hamishknight hamishknight is a code owner

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 @etcwilde @hamishknight