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

CMake build definitions #1624

Merged
merged 50 commits into from Nov 5, 2018

Conversation

Projects
None yet
5 participants
@jackgerrits
Copy link
Member

jackgerrits commented Oct 10, 2018

This PR add CMake build definitions. They do not include clr or csharp, but they do build on Windows and Linux. This attempt has gone down the route of as small a change as possible but still a step in the right direction.

Items still left to be done:

  • Create base makefile to perform CMake actions for discovery
  • Remove Automake
  • Remove extra work done to output both shared and static libs at the same time and just use normal CMake behavior
  • Update Readme with new workflows and to remove automake references
  • Test with and without vcpkg on Linux
  • Test and document Clang support
  • Test CMake options for Debug etc
  • Include Java and Python installs into CMake install
  • Add Num processors to cmake --build -- Not feasible
  • Change tests from custom_targets to proper CMake tests
  • Limit build configuration types to Debug and Release
  • Update versioning to work with CMake and Windows builds
  • Convert Java to CMake
  • Update Travis build to use CMake
    More:
  • Build doesn't seem to understand the number of processors.
  • Add targets for profiling, valgrind, static.
  • remove ffast-math from valgrind profile (fixing an inconsistency)
  • Fix static compile
  • Make it so install headers are automatically headers
  • Do we need disambiguators for else() in the vowpalwabbit directory?

Fixes #1635

@jackgerrits

This comment has been minimized.

Copy link
Member Author

jackgerrits commented Oct 12, 2018

I've updated the PR with the install step properly working now including:

  • Replacing values in the pkgconfig files
  • Generating both static and shared libs
  • Installing headers, libs and binaries
  • Offering options similar to those in the automake configure
  • Include headers for IDE generators
  • Replace package version in config.h file
  • NOTE: I am not generating or placing .la files like automake did, I am not sure if they are important

I am not sure how to determine if this brings us to automake parity but after running make install the outputs are of almost the same structure (see .la note above).

More testing is needed, but we are getting close to replacing 2 build systems with this change.

jackgerrits added some commits Oct 12, 2018

- Update build linux script for cmake
- Add cmake support for Java and Python
- Rewrite most of Python setup file for CMake
@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Oct 26, 2018

This pull request introduces 9 alerts when merging c0d19cb into a18d3ab - view on LGTM.com

new alerts:

  • 6 for Unused import
  • 1 for Unused local variable
  • 1 for Module is imported with 'import' and 'import from'
  • 1 for Variable defined multiple times

Comment posted by LGTM.com

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Oct 27, 2018

This pull request introduces 9 alerts when merging 437a1e1 into a18d3ab - view on LGTM.com

new alerts:

  • 6 for Unused import
  • 1 for Unused local variable
  • 1 for Module is imported with 'import' and 'import from'
  • 1 for Variable defined multiple times

Comment posted by LGTM.com

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Oct 30, 2018

Issues:

  1. Build doesn't seem to understand the number of processors.
  2. Add targets for profiling, valgrind, static.
  3. remove ffast-math from valgrind profile (fixing an inconsistency)
  4. Fix static compile
  5. Make it so install headers are automatically headers
  6. Do we need disambiguators for else() in the vowpalwabbit directory?
@@ -0,0 +1,55 @@
find_package(JNI)

This comment has been minimized.

@JohnLangford

JohnLangford Oct 30, 2018

Collaborator

@jmorra @deaktator can you look at the cmake build for java here?

@@ -0,0 +1,36 @@
if(NOT DEFINED PY_VERSION)

This comment has been minimized.

@JohnLangford

JohnLangford Oct 30, 2018

Collaborator

@Scott-Graham-Bose This is the relevant bit for python build changes.

@@ -0,0 +1,53 @@
add_subdirectory(unit_test)

This comment has been minimized.

@JohnLangford

JohnLangford Oct 30, 2018

Collaborator

@arielf These are the change to invocation of tests.

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Oct 30, 2018

@eisber you may also be interested in this patch.

@eisber

This comment has been minimized.

Copy link
Collaborator

eisber commented Oct 30, 2018

@jackgerrits

This comment has been minimized.

Copy link
Member Author

jackgerrits commented Oct 30, 2018

Disambiguators for else are not needed, they are used by CMake to warn if if-endifs do not match

jackgerrits added some commits Oct 30, 2018

@jackgerrits

This comment has been minimized.

Copy link
Member Author

jackgerrits commented Oct 30, 2018

@JohnLangford I believe I have resolved the issues we came across today

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Oct 30, 2018

This pull request introduces 6 alerts when merging 8c1013e into 05cca52 - view on LGTM.com

new alerts:

  • 2 for Local variable hides global variable
  • 1 for Unused import
  • 1 for No virtual destructor
  • 1 for Comparison result is always the same
  • 1 for Short global name

Comment posted by LGTM.com

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Oct 31, 2018

This pull request introduces 6 alerts when merging 8537b54 into f11d323 - view on LGTM.com

new alerts:

  • 2 for Local variable hides global variable
  • 1 for Unused import
  • 1 for No virtual destructor
  • 1 for Comparison result is always the same
  • 1 for Short global name

Comment posted by LGTM.com

This was referenced Nov 1, 2018

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Nov 1, 2018

This pull request introduces 6 alerts when merging 8537b54 into ad60f86 - view on LGTM.com

new alerts:

  • 2 for Local variable hides global variable
  • 1 for Unused import
  • 1 for No virtual destructor
  • 1 for Comparison result is always the same
  • 1 for Short global name

Comment posted by LGTM.com

jackgerrits added some commits Nov 1, 2018

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Nov 1, 2018

This pull request introduces 3 alerts when merging 7d1e2ff into 1ea72b2 - view on LGTM.com

new alerts:

  • 1 for Local variable hides global variable
  • 1 for Comparison result is always the same
  • 1 for Short global name

Comment posted by LGTM.com

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Nov 2, 2018

This pull request introduces 3 alerts when merging 163c58f into acd910e - view on LGTM.com

new alerts:

  • 1 for Local variable hides global variable
  • 1 for Comparison result is always the same
  • 1 for Short global name

Comment posted by LGTM.com

@JohnLangford

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Nov 3, 2018

This pull request introduces 3 alerts when merging f9554f4 into 72f6f43 - view on LGTM.com

new alerts:

  • 1 for Local variable hides global variable
  • 1 for Comparison result is always the same
  • 1 for Short global name

Comment posted by LGTM.com

@lokitoth lokitoth merged commit 2a8f497 into VowpalWabbit:master Nov 5, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Collaborator

JohnLangford commented Nov 5, 2018

This pull request introduces 3 alerts when merging 75ee3d0 into fb8d488 - view on LGTM.com

new alerts:

  • 1 for Local variable hides global variable
  • 1 for Comparison result is always the same
  • 1 for Short global name

Comment posted by LGTM.com

@jackgerrits jackgerrits deleted the jackgerrits:jagerrit/cmake_vw branch Jan 15, 2019

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