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.

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

Quote cmake include path #109

Merged
merged 1 commit into from Sep 17, 2022
Merged

Quote cmake include path #109

merged 1 commit into from Sep 17, 2022

Conversation

krzyzanowskim
Copy link
Contributor

quote include path that helps if path contains eg. a space

@milseman
Copy link
Contributor

@compnerd can you take a look?

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.

I think that this could use a better commit message - it isn't immediately clear what it means. Normally the include path would refer to the CMAKE_MODULE_PATH which shouldn't require quoting if handled properly IIRC. This is in the generated config file and the quoting does matter here.

@krzyzanowskim
Copy link
Contributor Author

refer to the CMAKE_MODULE_PATH which shouldn't require quoting if handled properly IIRC

long story short: swift won't build if checkout to path with space

When building swift-project using build-script if fails randomly here and there, and this is one place where it fail to include cmake macros from a path with spaces.

@compnerd
Copy link
Collaborator

@krzyzanowskim I think that you misunderstood me, I agree that this site should be quoted. I think that the "quote cmake include path" is a bit confusing is all. This is an include, but the quoting is applied to a parameter. The "CMake include path" is normally the CMAKE_MODULE_PATH, which is not being touched here and is confusing because you expect the diff to be different.

@compnerd
Copy link
Collaborator

@swift-ci please test

@compnerd
Copy link
Collaborator

I don't think that the commit message is worth keeping this open. Going to merge it.

@compnerd compnerd merged commit 252c0f0 into apple:main Sep 17, 2022
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.

None yet

3 participants