Prevent Python version mix and introduce NEST build option -Dwith-python=ON/OFF/2/3 #452

Merged
merged 7 commits into from Aug 17, 2016

Conversation

Projects
None yet
3 participants
@gtrensch
Contributor

gtrensch commented Aug 5, 2016

Proposed solution / workaround for issue #412.

@apeyser please can you have a look at it ?

gtr added some commits Aug 5, 2016

gtr
issue 412: - check for exact matching version of Python interpreter a…
…nd Python libraries

           - introduce CMake build option "-Dwith-python3=ON"
@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Aug 11, 2016

Contributor

Maybe an error if python3 == ON but python == OFF ? Or maybe we should just assume that people know what they're doing, since we may want to switch the default from python2 to python3 at some point, and such an error check would then make code complex.

So: 👍

Contributor

apeyser commented Aug 11, 2016

Maybe an error if python3 == ON but python == OFF ? Or maybe we should just assume that people know what they're doing, since we may want to switch the default from python2 to python3 at some point, and such an error check would then make code complex.

So: 👍

gtr
issue 412: remove 'cmake'-argument '-Dwith-python3' again; remove the…
… '-Dwith-python'-option 'path/to/python'; add new '-Dwith-python'-options 2 and 3 to set a specific python version
@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch Aug 16, 2016

Contributor

@apeyser: I removed the "path/to/python" option from "-Dwith-python". This is not sufficient anyway. It is much better to set all cmake built-in variables required for Python. I have described this in INSTALL. I also removed the "-Dwith-python3" cmake argument again. Instead of "path/to/python" a specific version can now be set with "-Dwith-python=ON/2/3/OFF". ON is the default. In that case cmake will chose a Python version (usually 2, the lowest installed). The cmake script ensures that all Python components are of exact same version. There is also a parameter check now.

Contributor

gtrensch commented Aug 16, 2016

@apeyser: I removed the "path/to/python" option from "-Dwith-python". This is not sufficient anyway. It is much better to set all cmake built-in variables required for Python. I have described this in INSTALL. I also removed the "-Dwith-python3" cmake argument again. Instead of "path/to/python" a specific version can now be set with "-Dwith-python=ON/2/3/OFF". ON is the default. In that case cmake will chose a Python version (usually 2, the lowest installed). The cmake script ensures that all Python components are of exact same version. There is also a parameter check now.

@gtrensch gtrensch changed the title from Prevent Python version mix and introduce NEST build option -Dwith-python3=ON/OFF to Prevent Python version mix and introduce NEST build option -Dwith-python=ON/OFF/2/3 Aug 16, 2016

INSTALL
+as an argument to `cmake`.
+
+`cmake` usually autodetects your Python installation.
+In some cases `cmake` v2.8.12 might not be able to localize the Python interpreter

This comment has been minimized.

@jougs

jougs Aug 16, 2016

Contributor

Do we need the version number here, i.e. are we sure that the problem only happens with this exact version? I think the sentence is just as clear without the version.

@jougs

jougs Aug 16, 2016

Contributor

Do we need the version number here, i.e. are we sure that the problem only happens with this exact version? I think the sentence is just as clear without the version.

This comment has been minimized.

@apeyser

apeyser Aug 17, 2016

Contributor

Isn't it cmake < 3.4?

@apeyser

apeyser Aug 17, 2016

Contributor

Isn't it cmake < 3.4?

pynest/README.md
@@ -21,6 +21,14 @@ PyNEST to `$(pyexecdir)`, which is often expanded as follows:
$(prefix)/lib{,64}/pythonX.Y/site-packages/nest
+
+To force to usage of a specific Python version pass

This comment has been minimized.

@jougs

jougs Aug 16, 2016

Contributor

"force to" → "force the"

@jougs

jougs Aug 16, 2016

Contributor

"force to" → "force the"

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 16, 2016

Contributor

👍 after fixing my minor comments. Many thanks for this contribution!

Contributor

jougs commented Aug 16, 2016

👍 after fixing my minor comments. Many thanks for this contribution!

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Aug 17, 2016

Contributor

Discussed w/ @gtrensch 👍
Just confirm that the install is correct in saying cmake == 2.8.12, or is a cmake < 3.4

Contributor

apeyser commented Aug 17, 2016

Discussed w/ @gtrensch 👍
Just confirm that the install is correct in saying cmake == 2.8.12, or is a cmake < 3.4

@gtrensch

This comment has been minimized.

Show comment
Hide comment
@gtrensch

gtrensch Aug 17, 2016

Contributor

Version number removed to "be on the safe side".

Contributor

gtrensch commented Aug 17, 2016

Version number removed to "be on the safe side".

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 17, 2016

Contributor

This fixes #412.

Contributor

jougs commented Aug 17, 2016

This fixes #412.

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