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

Improve conda support on windows #27

Merged
merged 18 commits into from
Dec 11, 2017
Merged

Conversation

fredboudon
Copy link
Contributor

No description provided.

@fredboudon fredboudon changed the title Improve conda support in qt4 and qhull modularity Improve conda support on windows Dec 5, 2017
@fredboudon
Copy link
Contributor Author

@pradal I change a number of tools to better take into account conda environment, in particular on windows.

Copy link
Contributor

@pradal pradal left a comment

Choose a reason for hiding this comment

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

@fredboudon Excellent. I have done some minor review. Mainly change CONDA_LIBRARYPREFIX to CONDA_LIBRARY_PREFIX.

CONDA_ENV = is_conda()
CONDA_PREFIX = conda_prefix()
CONDA_LIBRARYPREFIX = conda_library_prefix()
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace CONDA_LIBRARYPREFIX by CONDA_LIBRARY_PREFIX

Copy link
Contributor

Choose a reason for hiding this comment

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

Add also a new line

@@ -251,10 +251,16 @@ def locateQt4Command(env, command, qtdir) :
env['QTDIR'] = _detect(env)
# TODO: 'Replace' should be 'SetDefault'
# env.SetDefault(
qtinclude = os.path.join('$QTDIR', 'include')
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to explain why we search a dir with qt and qt/QtCore.
Later on these lines have to be understandable...

@@ -40,8 +40,8 @@ def depends(self):
def default(self):

if CONDA_ENV:
self._default['include'] = pj(CONDA_PREFIX,'include')
self._default['libpath'] = pj(CONDA_PREFIX,'lib')
self._default['include'] = pj(CONDA_LIBRARYPREFIX,'include')
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace CONDA_LIBRARYPREFIX by CONDA_LIBRARY_PREFIX

@@ -34,7 +34,11 @@ def __init__(self, config):
def default(self):

if CONDA_ENV:
self._default['bin'] = os.path.join(CONDA_PREFIX, 'bin')
if os.name == 'posix':
self._default['bin'] = os.path.join(CONDA_LIBRARYPREFIX, 'bin')
Copy link
Contributor

Choose a reason for hiding this comment

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

IDEM

@@ -56,8 +69,11 @@ def default(self):

# -- lets now look for decent include dirs --
if CONDA_ENV:
self._default['include'] = pj(CONDA_PREFIX, 'include')
self._default['lib'] = pj(CONDA_PREFIX, 'lib')
self._default['include'] = pj(CONDA_LIBRARYPREFIX, 'include')
Copy link
Contributor

Choose a reason for hiding this comment

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

CONDA_LIBRARY_PREFIX

# self._conda_build = False
# prefix = self._default['build_prefix'] = pj(self.config.dir[0],"build-scons")

prefix = self._default['build_prefix'] = pj(self.config.dir[0],"build-scons")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand. On conda, we want to put the lib, include and bin directly in the PREFIX and LIB_PREFIX and not in build_scons, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, see deploy. The cleanest solution is to build in build-scons and install on system. See install.py for thus

Copy link
Contributor

Choose a reason for hiding this comment

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

Deploy has always called scons in a build mode and not in install mode. I had some problem on Linux/Mac because if I build the lib in build-scons, libs are linked with other libs in the build-scons/lib and conda do not succeed t relocate it.
So we have to test this behavior. If it works, I am OK do move from scons build to scons install

@@ -44,7 +44,7 @@ def default(self):
self._default['flags'] = ''
self._default['defines'] = ''
if CONDA_ENV:
prefix = CONDA_PREFIX
prefix = CONDA_LIBRARYPREFIX
Copy link
Contributor

Choose a reason for hiding this comment

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

CONDA_LIBRARY_PREFIX

@@ -34,7 +34,11 @@ def __init__(self, config):
def default(self):

if CONDA_ENV:
base_dir = CONDA_PREFIX
if os.name == 'posix':
base_dir = CONDA_LIBRARYPREFIX
Copy link
Contributor

Choose a reason for hiding this comment

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

On posix, LIBRARY_PREFIX is not set. So why do you change these lines?

if os.name == 'posix':
self._default['libpath'] = pj(CONDA_PREFIX, 'lib')
else:
self._default['libpath'] = pj(CONDA_PREFIX, 'libs')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that for CONDA, Python is also set in the environment variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lib of python (python27.dll) is in the libs directory and is not set correctly by default by sconsx on windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. These lines are also correct outside conda build.

if env['WITH_QHULL'] :
def_qhull_inc = env['qhull_includes']
qhull_inc = pj(def_qhull_inc, 'libqhull')
if not os.path.exists(os.path.join(def_qhull_inc, "qhull_a.h")) and not os.path.exists(os.path.join(qhull_inc, "qhull_a.h")) :
Copy link
Contributor

Choose a reason for hiding this comment

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

Cut this line in two lines to ease. We just need to look at qhull_a.h, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed we look at qhull_a.h but 2 possible directory of header exists and are tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be clearer to use path.py for the path management. But I will do that on another PR

@fredboudon fredboudon merged commit 244fe9d into openalea:master Dec 11, 2017
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.

3 participants