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

Allow the CMake project to use system-wide installed dependencies #35

Merged
merged 6 commits into from
May 9, 2019
Merged

Allow the CMake project to use system-wide installed dependencies #35

merged 6 commits into from
May 9, 2019

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented May 6, 2019

Motivation and Context

Simplify workflow for developers who have some of the dependencies installed system-wide -- making full rebuild times shorter and allowing changes to dependent projects to be made without git submodule update (done either by accident or as part of setup.py) destroying them again.

Together with that a bunch of other minor things. Let me know if it's okay like this or I should submit these separately.

Note: I did not update any docs, as I'm not sure where to put these (or if the new CMake options are any useful for the general public).

How Has This Been Tested

Creating a custom CMake build folder with desired options specified and then directing setup.py to use that. While testing I realized a few further modifications could be made to setup.py to further simplify this workflow. These are however fully orthogonal to these changes, so I will submit them as a separate PR.

I suggest reviewing commit-by-commit as each commit message describes each change in more detail. Also, please point out all crimes against coding style etc. I made :)

Types of changes

  • Docs change / refactoring / dependency upgrade -- buildsystem update, in particular
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation. -- I think? Not sure, see above.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

Copy link
Collaborator

@msavva msavva left a comment

Choose a reason for hiding this comment

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

Thanks @mosra -- changes in PR look good to me. Left one minor nit comment about what seems to be a mis-indendation.
Checked default compilation on MacOS 10.14.1 through build.sh but didn't try using system-wide dependencies. I do think we should expose the option to build with system dependencies as that will help dev experience and build times.

src/cmake/dependencies.cmake Outdated Show resolved Hide resolved
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 7, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

mosra added 6 commits May 7, 2019 18:33
Fixes a warning related to GLVND on my machine.
This however means we have to bundle a few Find modules here, instead of
picking them up from the submodules directly.
Uncovered when switching to system dependencies, with the bundled ones it
wasn't complaining as the target already existed.
@mosra
Copy link
Collaborator Author

mosra commented May 7, 2019

Rebased on current master to fix conflicts with now-merged #29.

Copy link
Contributor

@erikwijmans erikwijmans left a comment

Choose a reason for hiding this comment

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

Builds for me on MacOS and ubuntu -- without system dependencies

@mathfac
Copy link
Contributor

mathfac commented May 7, 2019

Looks useful for project quality of life. Very clean PR. Thank you for introducing editorconfig, we will consider to use that for habitat-api as well.

@mosra mosra merged commit 5e4cfbc into facebookresearch:master May 9, 2019
@mosra mosra mentioned this pull request May 9, 2019
11 tasks
eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
…cebookresearch#35)

* Add .editorconfig.

* CMake: let Magnum find OpenGL.

Fixes a warning related to GLVND on my machine.

* CMake: make it possible to use system packages for most dependencies.

This however means we have to bundle a few Find modules here, instead of
picking them up from the submodules directly.

* esp/bindings: minor update to make things work with stable pybind11.

* esp/nav: forward-declare a struct as a struct.

* esp/gfx: properly find the Application library.

Uncovered when switching to system dependencies, with the bundled ones it
wasn't complaining as the target already existed.
@WYXG233 WYXG233 mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants