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

Various CMake fixes and cleanup #360

Merged
merged 4 commits into from
Jan 16, 2019
Merged

Various CMake fixes and cleanup #360

merged 4 commits into from
Jan 16, 2019

Conversation

jamiesnape
Copy link
Contributor

@jamiesnape jamiesnape commented Jan 16, 2019

Do not set redundant macOS-related policy; CMP0042 is enabled by virtue of requiring CMake 3.0 on macOS. Do not require a C compiler; there is no C code. Fix typo in CMAKE_INSTALL_FULL_LIBDIR variable name; there is no such variable CMAKE_INSTALL_LIBDIR_FULL. Remove redundant set of CMAKE_SHARED_MODULE_PREFIX; there are no modules being built.


This change is Reviewable

@jamiesnape
Copy link
Contributor Author

Note apparently master is broken on Mac per https://travis-ci.org/flexible-collision-library/fcl/builds/480449098.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Thanks, @jamiesnape -- :lgtm: with a few minor comments. Also, would you please edit the PR description to summarize the changes you're making here and why?

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


CMakeLists.txt, line 4 at r1 (raw file):

# vi: set ft=cmake :

# Copyright (c) 2012, Willow Garage, Inc.

BTW why add an old Willow Garage copyright? Are you thinking this was inadvertently left off long ago?

Consider also adding
`# Copyright (c) 2019, Toyota Research Institute, Inc.


CMakeModules/CompilerSettings.cmake, line 4 at r1 (raw file):

# vi: set ft=cmake :

# Copyright (c) 2013, Willow Garage, Inc.

BTW same comment

Copy link
Contributor Author

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


CMakeLists.txt, line 4 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW why add an old Willow Garage copyright? Are you thinking this was inadvertently left off long ago?

Consider also adding
`# Copyright (c) 2019, Toyota Research Institute, Inc.

Yes, according to git. If you wanted to add a TRI copyright, 2016 looks to be correct date.

Copy link
Contributor Author

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

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

(Also I would encourage you to accept un-squashed PR's on occasion; there are numerous small issues with the CMake that are not especially related, and making separate micro-PR's seems inefficient for CI and reviewers' time.)

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Thanks, Jamie. I'll merge-commit this one so that the individual commits are preserved.

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

2 participants