Skip to content

Hunterize libigl, attempt 2#1242

Merged
jdumas merged 15 commits intolibigl:devfrom
rbsheth:rbsheth_add_hunter
Jul 5, 2019
Merged

Hunterize libigl, attempt 2#1242
jdumas merged 15 commits intolibigl:devfrom
rbsheth:rbsheth_add_hunter

Conversation

@rbsheth
Copy link
Copy Markdown
Contributor

@rbsheth rbsheth commented Jun 25, 2019

Original PR with missing branch here: #1202

This PR adds basic support for the Hunter package manager in libigl. Because libigl is already following modern CMake practices, the changes needed to make this package Hunter-ready are minimal.

Changes:

  1. Add HunterGate.cmake and corresponding CMake code in CMakeLists.txt
  2. Pull Eigen from Hunter
  3. Add HUNTER_ENABLED flag and default to OFF to preserve current behavior.

Once this change is merged, a new (minor?) GitHub release is needed to generate a source tarball that Hunter can pull in.

Check all that apply (change to [x])

  • All changes meet libigl style-guidelines.
  • Adds new .cpp file.
  • Adds corresponding unit test.
  • Adds corresponding python binding.
  • This is a minor change.

@rbsheth rbsheth mentioned this pull request Jun 25, 2019
Closed
5 tasks
jdumas
jdumas requested changes Jun 25, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jdumas jdumas left a comment

Choose a reason for hiding this comment

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

Looks good, but I think you forgot a change to libigl-config.cmake.in you had in the other PR?
Also I've questions about the two MSVC flags you enabled.

rbsheth
rbsheth commented Jun 25, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@rbsheth rbsheth left a comment

Choose a reason for hiding this comment

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

@jdumas In the dev branch, I noticed that the way Eigen is found matches exactly what Hunter needs in libigl-config.cmake.in, so I didn't need to port that change over.

@rbsheth
Copy link
Copy Markdown
Contributor Author

rbsheth commented Jun 26, 2019

@jdumas Any ideas on this Travis failure? https://travis-ci.org/libigl/libigl/jobs/550530668

Looks like ccache is missing?

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Jun 26, 2019

Yes. I'm waiting for another maintainer to approve #1239 (I can't merge it myself) which fixes the CI issues. Once this is done we can merge this PR.

jdumas and others added 3 commits June 26, 2019 17:48
@jdumas
Add conda environment file for boost (helpful for Windows users). (li…
7c784ab 
…bigl#1239)

* Add conda environment file for boost (helpful for Windows users).

* Try to update Travis image for macOS.
update IGL_DEPRECATED to handle clang (e.g. on macOS) as well.
87a50b8
@jdumas
Merge pull request libigl#1244 from BruegelN/update-IGL_DEPRECATED-fo…
31c4eb2 
…r-clang

update IGL_DEPRECATED to handle clang (e.g. on macOS) as well.
@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Jun 28, 2019

@rbsheth I'm trying to merge the tip of the dev branch with your PR branch to trigger a new build on Travis, but it doesn't look like you've given us write access to the branch when you open your PR. Could you see to it?

@rbsheth
Copy link
Copy Markdown
Contributor Author

rbsheth commented Jul 1, 2019

@jdumas Invited you to collaborate

@jdumas
Copy link
Copy Markdown
Collaborator

jdumas commented Jul 5, 2019

@rbsheth do you mind if I squash and merge instead of just merging? Or will it affect your fork in hunter for future versions?

@rbsheth
Copy link
Copy Markdown
Contributor Author

rbsheth commented Jul 5, 2019

@jdumas Squash merge should be fine, I can work with that for Hunter.

@jdumas jdumas merged commit 84c886b into libigl:dev Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdumas jdumas jdumas approved these changes

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

@rbsheth @jdumas @BruegelN