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

Add support for Conan #508

Merged
merged 13 commits into from Jul 3, 2018

Conversation

Projects
None yet
5 participants
@madebr
Copy link
Collaborator

madebr commented Jun 7, 2018

These commits allow building OpenRW using Conan.

Conan is a C/C++ package manager, which means that it is able to download/build the dependencies.

Overview of the patches:

  • The patches modify the CMake build script such that building with or without conan gives no change in behavior. Differences between static and shared libraries are abstracted out as much as possible.
  • Installing some conan packages requires sudo privilege (it needs some system dependencies). For this a change to the docker script was needed.
  • I have added a conan test to travis-ci
  • AppVeyor is using conan instead of the openrw-windows-dependencies repo
  • A convenience script for Visual Studio developers to create a Visual Studio solution. Using this script, Visual Studio will not build the rwviewer because of bincrafters/community#313. However, this pr does not block because I have disabled the viewer in this script.

Notes:

TODO:
A prebuilt version of ffmpeg and SDL2 is not available on bintray because the profiles give it non-default options. Therefore it has to be built, which takes time (this needs to be done only once). On AppVeyor this can be dealt with using the cache. The conan docker instance cannot make use of the cache (without making the docker images more complicated) and rebuild these libraries.

I've asked upstream to include these:

Prebuilt debug qt 5.11 on conan: bincrafters/community#313: This pr does not block on this issue

If windows will use conan, the repo openrw-windows-dependencies can be removed


Background

Conan requires one extra step before configuring and building OpenRW: installing the dependencies and generating a file that can be included by CMake that contains the paths etc.

  1. Downloading (installing) all dependencies:
    • dependencies are listed in the conanfile.py file.
    • differences between platforms are handled by profiles. A profile can modify properties of the dependencies, build environment etc.
    • run conan install $OPENRW_DIR -pr $OPENRW_DIR/scripts/conan/linux --build missing
    • The --build missing is required only if some dependencies are not available in the local cache and online.
  2. Building OpenRW
    • OpenRW is built using the conan packages only if USE_CONAN evaluates to true.
    • Some libraries (such as Boost, OpenAL and SDL2) require special handling if it is built as a static library. conanfile.py contains the required configuration options.
    • run conan build $OPENRW_DIR OR run cmake $OPENRW_DIR -DUSE_CONAN=1 -DBOOST_STATIC=True -DSDL2_STATIC=True
@madebr

This comment has been minimized.

Copy link
Collaborator

madebr commented Jun 7, 2018

Please proofread this so it can be copied to the wiki.

Build OpenRW with Conan

See https://docs.conan.io/en/latest/introduction.html for more detailed information

Install conan on system

Requirements:

  • python 3 & pip3
  • conan
# Install & configure conan
pip3 install --user conan
conan user
# install bincrafters repo (=community packaging initiative)
conan remote add bincrafters https://api.bintray.com/conan/bincrafters/public-conan

Keep conan updated

Packages for conan might use new features of Conan. So keep your installation updated.
Update conan first, before opening an issue about it.

pip3 install --user --upgrade conan

Build OpenRW using conan

Linux

cd openrw && mkdir -p build/conan && cd build/conan
conan install ../.. -pr ../../scripts/conan/linux --build missing
conan build ../..

Windows

cd openrw && mkdir -p build\conan && cd build\conan
conan install ../.. -pr ../../scripts/conan/windows --build missing
conan build ../..

Create a visual studio solution that supports Release and Debug:

cd openrw && mkdir -p build\vs_solution && cd build\vs_solution
python ..\..\scripts\conan\create_vs_solution.py

MacOS

UNTESTED

cd openrw && mkdir -p build/conan && cd build/conan
conan install ../.. -pr ../../scripts/conan/darwin --build missing
conan build ../..

@madebr madebr force-pushed the madebr:conan branch from de6d5b7 to 04e6fce Jun 9, 2018

@madebr madebr force-pushed the madebr:conan branch from 04e6fce to 7d2da28 Jun 14, 2018

@madebr madebr referenced this pull request Jun 14, 2018

Open

How to use? #4

@madebr madebr force-pushed the madebr:conan branch from 7ef0515 to 9d9923c Jun 14, 2018

@madebr madebr force-pushed the madebr:conan branch from 9d9923c to 9919fe5 Jun 14, 2018

@darkf

This comment has been minimized.

Copy link
Collaborator

darkf commented Jun 15, 2018

@danhedron How are we feeling about this now?

@JayFoxRox

This comment has been minimized.

Copy link
Collaborator

JayFoxRox commented Jun 18, 2018

  • "(Python 2 is supported but support for that version will cease soon)" I'd turn this into "" for the wiki as it's not important to the user, but prevents idiots from changing the steps

  • I'd also get rid of "Prerequisite:" or change it to "Requirements"; I feel either of these words don't add anything + some users might not know what they even mean = confusing

  • Where is $OPENRW_DIR exported? It wasn't immediately clear to me that the "Background" section did not have to be ran and only serves as explanation. I'd ditch that section entirely and link to official Conan docs if absolutely necessary.
    Keeping such docs in the PR description is probably better, as it's information for maintainers, not users.


Overall I believe this will be tough to review (due to lack of experience with Conan) and probably doesn't add much (the old system works fine for most).
Personally I also think that distribution of deps should be solved by the platforms preferred package manager, with the build system integrating these things nicely. However, I also see some value in this - I just don't think it's the right fit for OpenRW at this point in time.

Other maintainers might have a different view. Also the code has been written, so might as well integrate it, if "Conan is currently in heavy development" doesn't imply that this will need a ton of updates which delay other PRs. I don't think we will add many new deps to OpenRW either, so fortunately that won't cause extra work (which would be a major issue).

@madebr

This comment has been minimized.

Copy link
Collaborator

madebr commented Jun 18, 2018

Updated the text for the wiki

I will post the background section here under for later reference.

Background

Conan requires one extra step before configuring and building OpenRW: installing the dependencies and generating a file that can be included by CMake that contains the paths etc.

  1. Downloading (installing) all dependencies:
    • dependencies are listed in the conanfile.py file.
    • differences between platforms are handled by profiles. A profile can modify properties of the dependencies, build environment etc.
    • run conan install $OPENRW_DIR -pr $OPENRW_DIR/scripts/conan/linux --build missing
    • The --build missing is required only if some dependencies are not available in the local cache and online.
  2. Building OpenRW
    • OpenRW is built using the conan packages only if USE_CONAN evaluates to true.
    • Some libraries (such as Boost, OpenAL and SDL2) require special handling if it is built as a static library. conanfile.py contains the required configuration options.
    • run conan build $OPENRW_DIR OR run cmake $OPENRW_DIR -DUSE_CONAN=1 -DBOOST_STATIC=True -DSDL2_STATIC=True
@madebr

This comment has been minimized.

Copy link
Collaborator

madebr commented Jun 18, 2018

What this pr adds:

  • prebuilt packages, release and debug (including boost and qt, which is not provided by openrw-windows-dependencies)
  • correct Visual Studio solution (where switching between Debug and Release works, tested on my win10 system) This did not work previously (madebr/openrw-windows-dependencies#4)
  • conan is in heavy development: was maybe an exaggeration. What I meant to say is that new packages might use new features of conan, so updating your conan might be needed.
  • we offload building the dependencies to professional packagers
@JayFoxRox

This comment has been minimized.

Copy link
Collaborator

JayFoxRox commented Jun 18, 2018

More thoughts:

  • Move the "Background" to the first post so everyone can find it
  • "conan" is not a requirement, as the first line installs it. It also implies that users should install it however they want - which may not be using pip, which is referenced in the upgrade line. However, pip3 is a requirement.
  • Will this PR replace the openrw-windows-dependencies for good, or is it meant as an alternative?
    • What prevents us from ditching openrw-windows-dependencies, are there any issues why conan can't be the default for Windows (technical / political / social)?
  • Regarding "we offload building the dependencies to professional packagers": and so does homebrew, each distros package manager and vcpkg. Which is not really a benefit of conan imo. It's more of a political decision who takes care of it: the platform maintainers or a third party.
@madebr

This comment has been minimized.

Copy link
Collaborator

madebr commented Jun 18, 2018

  1. Moved Background to the first post
  2. python3 comes with pip3: compiling python from source, also creates a pip3 executable. Though, I added it to the requirements.
  3. By switching to conan, and ditching openrw-windows-dependencies, we know exactly what version of dependencies we are using. openrw-windows-dependencies is a poor mans package manager. Adding this support for conan does not exclude support for vcpkg, hunter or compiling them yourselves.
  4. It's also a matter of trust.
@danhedron
Copy link
Member

danhedron left a comment

LGTM; I've mentioned before that I don't like the rwdep:: style of wrapping targets, but that's a minor issue.

set(ENABLE_SANITIZERS "" CACHE STRING "Enable selected sanitizer.")
option(BOOST_STATIC "Link against static Boost libraries")

option(USE_CONAN "Use Conan as packet manager")

This comment has been minimized.

@danhedron

danhedron Jun 21, 2018

Member

*package

windows_profile = openrw_path / 'scripts' / 'conan' / 'windows'
cmake_generator_lookup = {
2015: 'Visual Studio 14 2015',
2017: 'Visual Studio 15 2017',

This comment has been minimized.

@danhedron

danhedron Jun 21, 2018

Member

Doesn't VS 2017 have native CMake support?

This comment has been minimized.

@madebr

madebr Jun 21, 2018

Collaborator

Yes, it has. See this link
I don't see immediately a way to run conan there.

@madebr

This comment has been minimized.

Copy link
Collaborator

madebr commented Jun 21, 2018

The main rationale for introducing rwdep::XXX targets is because the targets of the cmake modules do not support DEBUG and RELEASE, which is a requirement for Visual Studio support.

@danhedron

This comment has been minimized.

Copy link
Member

danhedron commented Jun 25, 2018

Sorry for the delay in getting this merged. I've been trying to catch up with the backlog on smaller (but still pretty big…) PRs.

@madebr

This comment has been minimized.

Copy link
Collaborator

madebr commented Jun 25, 2018

I have a local patch to get rid of the rwdeps. I have not pushed it yet because I have not tested it enough.

@danhedron danhedron merged commit caa1e1e into rwengine:master Jul 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ShFil119

This comment has been minimized.

Copy link
Member

ShFil119 commented on 4f98e36 Jul 13, 2018

It causes warning, can we somehow mute it down?

This comment has been minimized.

Copy link
Collaborator

madebr replied Jul 13, 2018

Just replace it with a FIXME.

@ShFil119

This comment has been minimized.

Copy link
Member

ShFil119 commented Dec 15, 2018

Wiki lacks info about using conan deps on linux.

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