Skip to content

Add support to CMake to build swift-argument-parser during SwiftPM bootstrapping #139

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

Merged
merged 1 commit into from
May 12, 2020

Conversation

owenv
Copy link
Contributor

@owenv owenv commented May 11, 2020

This is a fairly early attempt at integrating this repo into the SwiftPM bootstrapped build, someone better at CMake than I am should probably review :)

@natecook1000
Copy link
Member

cc @compnerd ?

Copy link
Collaborator

@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.

Seems good except for the unnecessary compatibility.

CMakeLists.txt Outdated

include(CTest)

if(CMAKE_VERSION VERSION_LESS 3.16 AND CMAKE_SYSTEM_NAME STREQUAL Windows)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed? The minimum for this project is currently 3.16. I think that we can ignore the <3.16 compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, this was unnecessary, I dropped it

@@ -2,3 +2,9 @@ add_subdirectory(ArgumentParser)
if(BUILD_TESTING)
add_subdirectory(ArgumentParserTestHelpers)
endif()

set_property(GLOBAL APPEND PROPERTY ARGUMENTPARSER_EXPORTS ArgumentParser)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use ArgumentParser_EXPORTS but, that’s a purely style thing, it has 0 bearing on anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, I'm not that familiar with CMake style conventions. I updated it since I was already making changes

@owenv owenv force-pushed the cmake-bootstrap branch from 16a3dc2 to 8fc9b43 Compare May 11, 2020 21:53
@owenv owenv force-pushed the cmake-bootstrap branch from 8fc9b43 to 8fcb6d4 Compare May 11, 2020 22:00
@owenv
Copy link
Contributor Author

owenv commented May 11, 2020

@compnerd thanks for reviewing!

@natecook1000
Copy link
Member

@swift-ci Please test

@compnerd
Copy link
Collaborator

@owenv my pleasure!

@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000 natecook1000 merged commit 7fc05c0 into apple:master May 12, 2020
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