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

Update source code with c++11 and python 2/3 compatibility for Ubuntu 17.10 #74

Closed
wants to merge 67 commits into from

Conversation

sildeag
Copy link

@sildeag sildeag commented Mar 2, 2018

Qt5 means Qt5.6 to Qt5.10 from launchpad.
Python 2/3 means for Qt4 and all other operating system same as before:
./waf configure --with-python-bindings --with-tests --with-asserts or
python 3 ./waf configure --with-python-bindings --with-tests --with-asserts
For Ubuntu or possibly other Linux platforms:
./waf configure --with-python-bindings --with-tests --with-asserts --with-gaia-qt5 or
python 3 ./waf configure --with-python-bindings --with-tests --with-asserts --with-gaia-qt5

--with-cyclops still being worked on.

Copy link
Member

@dbogdanov dbogdanov left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! As your contribution is larger than 20 lines of code can you submit a CLA form?

Some changes are necessary (see comments). Also better description for this pull request itself (mention updates for all dependencies, including waf, eigen)
Some thoughts:

  • It is not clear, we should update Eigen if it isn't entirely necessary.
  • Will older versions of swig work?
  • Remove hardcoded paths for GCC 7.2
  • Support for Intel MKL should be optional (add a configuration flag, otherwise, remove)
  • There are some unnecessary files (waflib)

@@ -0,0 +1,8 @@
argv = ['./waf', 'configure', '--with-python']
Copy link
Member

Choose a reason for hiding this comment

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

This file should be removed

README.md Outdated
@@ -16,7 +16,7 @@ Dependencies:
* Qt >= 4.5
* libYAML >= 0.1.1
* Python >= 2.4
* SWIG >= 1.3.31
* SWIG >= 3.0.10
Copy link
Member

Choose a reason for hiding this comment

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

Would still work with older swig versions? Ubuntu 14.04 LTS is 2.0.11.

Copy link
Author

Choose a reason for hiding this comment

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

There is no swig limitation other than you have already had issues with.

gaia.txt Outdated
@@ -0,0 +1,286 @@
Waf: Entering directory `/home/gordon/gaia/build'
Copy link
Member

Choose a reason for hiding this comment

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

Remove this fille

Copy link
Author

Choose a reason for hiding this comment

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

Okay

@@ -0,0 +1,1702 @@
#############################################################################
Copy link
Member

Choose a reason for hiding this comment

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

Remove, as this file can be generated from Gaia2lib.pro

Copy link
Author

Choose a reason for hiding this comment

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

Okay

@@ -4,63 +4,131 @@
// Copyright (C) 2008 Gael Guennebaud <gael.guennebaud@inria.fr>
// Copyright (C) 2007-2011 Benoit Jacob <jacob.benoit.1@gmail.com>
//
// Eigen is free software; you can redistribute it and/or
Copy link
Member

Choose a reason for hiding this comment

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

To keep track of things, which version has Eigen been updated to and what is the reason for the update?

Copy link
Author

Choose a reason for hiding this comment

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

The Eigen version 3.3.4 was mentioned and it was for c++11 compatibility.

src/wscript Outdated
@@ -119,6 +126,67 @@ def build_tools(bld):
use = ['gaia2'] + bld.env['USELIB'])

# Cyclops server
if sys.platform.startswith('linux') and gaia_qt5:
Copy link
Member

Choose a reason for hiding this comment

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

There's lots of duplicated code. It may be good to shorted the code and assign a features variable first (to qt5 qt5network cxx cxxprogram cyclops or qt4 cxx cxxprogram cyclops depending on gaia_qt5) and then run the generic build command.

Copy link
Author

Choose a reason for hiding this comment

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

I duplicated the code in the wscript files because I can only test on Ubuntu and I tried to leave build unchanged for other operating environments - the c++ source code changes likely will work for them too with the gnu compiler. Any changes/improvements recommended are okay with me. I still have to get the Cyclops option to compile without errors. The only other potential issue I am aware of is the location of the python directory eg. for 2.7 it is /usr/local/lib/python2.7/dist-packages. For python 3.6 it is /usr/local/lib/python3/dist-packages and I had fixed to /usr/local/lib/python3.6/dist-packages but wanted that to be more dynamic.

Copy link
Author

Choose a reason for hiding this comment

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

Now use conditional build_tools_qt4 method and build_tools_qt5 method.

src/wscript Outdated
target = 'tools/cyclopsmaster',
includes = [ '.', 'metrics', '3rdparty', 'tools/cyclops_server/' ],
use = ['gaia2'] + bld.env['USELIB'])
# cyclops init scripts
if bld.env['WITH_CYCLOPS']:
Copy link
Member

Choose a reason for hiding this comment

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

Make sure that build commands aren't run twice in the case of linux.

Copy link
Author

Choose a reason for hiding this comment

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

.waf runs some cyclops compiles twice and there is no easy way to avoid that.

src/wscript_new Outdated
@@ -0,0 +1,210 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

Remove this file, keep all updates in src/wscript

waf
VERSION="1.8.21"
REVISION="71bc2f1f9d6d313df7b3e05a55143f91"
GIT="x"
VERSION="2.0.6"
Copy link
Member

Choose a reason for hiding this comment

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

To document changes better, make sure to mention waf update to 2.0.6 in the pull request description.

Copy link
Author

Choose a reason for hiding this comment

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

I've included release notes for Linux in my README.md. waf 1.9 or better is required for Python2 or Python3.

waflib/Build.py Outdated
@@ -0,0 +1,1474 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

The waflib/* files should not be necessary. Remove

@sildeag
Copy link
Author

sildeag commented Mar 3, 2018

Will comply to all requested changes and send in CLA form.

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