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 paths when using Ananaconda #20

Closed
wants to merge 2 commits into from

Conversation

jlmaurer
Copy link

@jlmaurer jlmaurer commented May 15, 2020

  • Change FindCython.cmake search order so that it finds the conda version of cython
  • Add -DCMAKE_PREFIX_PATH=$CONDA_PREFIX to installation command when using anaconda
  • Without these changes, Fringe can get the system versions of these two packages
  • This PR fixes Fringe cmake can't find GDAL #9.

@dbekaert dbekaert requested a review from piyushrpt May 15, 2020 14:25
@rtburns-jpl
Copy link
Member

Alternatively you can invoke cython with python -m cython, that's what I do in the cmake files for isce2/3. That way it's guaranteed to work with the python version you're using
https://github.com/isce-framework/isce2/blob/master/.cmake/FindCython.cmake

@rtburns-jpl
Copy link
Member

Also, when you're using anaconda, I recommend passing -DCMAKE_PREFIX_PATH=$CONDA_PREFIX to cmake. This has the added benefit of finding other packages as well, and you don't have to remember the name of the active env

@jlmaurer
Copy link
Author

@rtburns-jpl and @piyushrpt, do you recommend I add the '''-DCMAKE_PREFIX_PATH=$CONDA_PREFIX''' argument to the current PR? This would I think also require adding an export command for the CONDA_PREFIX.

@rtburns-jpl
Copy link
Member

You'd provide -DCMAKE_PREFIX_PATH=$CONDA_PREFIX as an arg to cmake, i.e. alongside -DCMAKE_INSTALL_PREFIX=... - no need to export. I think this is generally preferable to setting *_ROOT variables.

@rtburns-jpl
Copy link
Member

Oh, I see what you mean about the CONDA_PREFIX export - that variable is automatically set by conda to your currently active environment, so no need to export it yourself :)

@jlmaurer
Copy link
Author

@rtburns-jpl Got it. I'll add the command arg to the install.md example.

@piyushrpt
Copy link
Member

When using conda compilers, you dont need to specify CXX

CXX=${CXX} 

is redundant

@@ -81,6 +81,9 @@ In the build folder
#On Linux
> CXX=g++ cmake -DCMAKE_INSTALL_PREFIX=../install ../src/fringe

# with anaconda (on Linux)
> CXX=g++ cmake -DCMAKE_INSTALL_PREFIX=../install ../src/fringe -DCMAKE_PREFIX_PATH=$CONDA_PREFIX
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer adding the CONDA_PREFIX as a solution just like the GDAL one, in a later section below. I havent had to do this for GDAL / cython on couple of different setups.

@@ -81,6 +81,9 @@ In the build folder
#On Linux
> CXX=g++ cmake -DCMAKE_INSTALL_PREFIX=../install ../src/fringe

# with anaconda (on Linux)
> CXX=g++ cmake -DCMAKE_INSTALL_PREFIX=../install ../src/fringe -DCMAKE_PREFIX_PATH=$CONDA_PREFIX

#If "conda install gxx_linux-64 / clangxx_osx-64"
> CXX=${CXX} cmake -DCMAKE_INSTALL_PREFIX=../install ../src/fringe
Copy link
Member

Choose a reason for hiding this comment

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

CXX=${CXX} is redundant when using conda compilers since these are automatically set when the environment is activated. I would remove that.

@@ -99,3 +102,6 @@ export PYTHONPATH=$PYTHONPATH:path-to-install-folder/python

export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:path-to-install-folder/lib
```
If using within an Anaconda environment:
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to make this a sub-section to capture the quirks that you have to deal with, especially for conda.

@@ -99,3 +102,6 @@ export PYTHONPATH=$PYTHONPATH:path-to-install-folder/python

export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:path-to-install-folder/lib
```
If using within an Anaconda environment:
export GDAL_DIR=base-anaconda-directory/envs/fringe/
Copy link
Contributor

@scottstanie scottstanie Sep 11, 2020

Choose a reason for hiding this comment

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

I was thinking of also making a small PR on this issue that I ran into- I ended up changing it at during the cmake command by adding

GDAL_DIR=`gdal-config --prefix`  cmake -DCMAKE_INSTALL_PREFIX=../install ..

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, was this due to using a system installation of gdal? In my experience I always use gdal from conda so -DCMAKE_PREFIX_PATH=$CONDA_PREFIX is sufficient, but this is a neat trick in the general case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you are right! I had mixed it up with another setup that didn't have the anaconda gdal. The -DCMAKE_PREFIX_PATH=$CONDA_PREFIX was indeed sufficient for the conda gdal install.

@hfattahi hfattahi mentioned this pull request Aug 3, 2021
@hfattahi
Copy link
Contributor

hfattahi commented Aug 4, 2021

Hi @jlmaurer, The #51 fixes the build on ci and also on local linux machine with conda. As mentioned here and in #51 we don't want to change the order for cython. My suggestion is to update this PR by removing the change to CMake for finding cython and with special cases where we specifically need to enforce such as the following:

-DCYTHON_EXECUTABLE=$CONDA_PREFIX/bin/cython

Or we can just close this PR and update the installation in a separate PR. Let me know what makes better for you.

@hfattahi
Copy link
Contributor

hfattahi commented Aug 7, 2021

@jlmaurer I'll close this PR for now as I'm going to rename the master to main. Feel free to re-open and update please if you think it is still needed.

@hfattahi hfattahi closed this Aug 7, 2021
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.

Fringe cmake can't find GDAL
5 participants