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: correctly include yaml-cpp #3558

Merged
merged 2 commits into from
Mar 12, 2018
Merged

cmake: correctly include yaml-cpp #3558

merged 2 commits into from
Mar 12, 2018

Conversation

mossheim
Copy link
Contributor

@mossheim mossheim commented Mar 4, 2018 •
edited
Loading

Include its dirs last for libsclang, and use the YAMLCPP_LIBRARY variable for linking.

Fixes #3557.

cc @Apteryks. This works for me on macOS, using v0.6.1 of yaml-cpp.

Edit - oops, think I broke linking when SYSTEM_YAMLCPP=OFF. will keep investigating.

@mossheim
cmake: correctly include yaml-cpp
f82cec5 
Include its dirs last for libsclang, and use the YAMLCPP_LIBRARY variable for linking
@mossheim mossheim added comp: build CMake build system comp: 3rd party 3rd party libraries/dependencies labels Mar 4, 2018
@mossheim mossheim mentioned this pull request Mar 4, 2018
Closed
@mossheim
Copy link
Contributor Author

mossheim commented Mar 5, 2018

AFAICT I can correctly build with SYSTEM_YAMLCPP on or off now. I'm cleaning in between each build & reconfigure. I feel a bit weird about setting the YAMLCPP_LIBRARY and YAMLCPP_INCLUDE_DIR options like this; it reduces complexity in other areas though.

@mossheim mossheim mentioned this pull request Mar 5, 2018
Closed
Apteryks
Apteryks approved these changes Mar 5, 2018
View reviewed changes
Copy link
Contributor

@Apteryks Apteryks left a comment

Choose a reason for hiding this comment

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

Seems to work as intended. I could build with both -DSYSTEM_YAMLCPP=on and -DSYSTEM_YAMLCPP=off. Thanks for the quick fix!

@mossheim
Copy link
Contributor Author

mossheim commented Mar 6, 2018

Seems to work as intended. I could build with both -DSYSTEM_YAMLCPP=on and -DSYSTEM_YAMLCPP=off. Thanks for the quick fix!

Great, I'm glad it works!

@mossheim
Copy link
Contributor Author

mossheim commented Mar 10, 2018 •
edited
Loading

@dvzrv reported also that this works (#3557 (comment)). I think this is OK to merge now.

@patrickdupuis patrickdupuis merged commit e77d9fe into supercollider:develop Mar 12, 2018
@mossheim mossheim deleted the issue/3557 branch March 12, 2018 00:58
@dvzrv
Copy link
Member

dvzrv commented Mar 25, 2018

Hmm, was this not included in 3.9.2?

@Apteryks
Copy link
Contributor

Seems it didn't make it yet.

@patrickdupuis
Copy link
Contributor

This was merged into the 'develop' branch and will be included in SC 3.10.

@dvzrv
Copy link
Member

dvzrv commented Mar 25, 2018

@patrickdupuis thanks. I'll keep the patches around for a little longer then ;-)

@nhthn nhthn added this to the 3.10 milestone Mar 31, 2018
@mossheim mossheim mentioned this pull request Mar 31, 2018
Merged
patrickdupuis added a commit that referenced this pull request Mar 31, 2018
@patrickdupuis
Merge pull request #3619 from brianlheim/port-3558
1d037e6 
Rebase #3558 on 3.9
@mossheim
Copy link
Contributor Author

mossheim commented Apr 3, 2018 •
edited
Loading

@dvzrv @Apteryks - since we are going to release a 3.9.3 soon (there was a regression from 3.9.1 to 3.9.2), I ported this fix to the 3.9 branch in #3619. It will be available for 3.9.3. :) Sorry that we missed getting it in 3.9.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Apteryks Apteryks Apteryks approved these changes

@joshpar joshpar joshpar approved these changes

Assignees
No one assigned
Labels
comp: build CMake build system comp: 3rd party 3rd party libraries/dependencies
Projects
None yet
Milestone
3.10
Development

Successfully merging this pull request may close these issues.
6 participants
@mossheim @dvzrv @Apteryks @patrickdupuis @joshpar @nhthn