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

Refactor python module installer and add install-python target to CMake #1375

Merged
merged 13 commits into from Mar 27, 2019

Conversation

@akohlmey
Copy link
Member

akohlmey commented Mar 22, 2019

Summary

This pull request provides some refactoring of the LAMMPS python module installation script to improve platform neutrality and include it cleanly into both, conventional and CMake based builds

Related Issues

closes #1373

Author(s)

Axel Kohlmeyer (Temple U)

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

The command line flags for manually running the Python module install script have changed. However suitable help messages and consistency checks on arguments have been added.

Implementation Notes

The CMake old configuration made (part of) the python module installation dependent on enabling the PYTHON package. This makes this independent and CMake builds should behave similar to conventional make now.

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted. Delete lines that don't apply

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

Put any additional information here, attach relevant text or image files, and URLs to external sites (e.g. DOIs or webpages)

akohlmey added some commits Mar 22, 2019

refactoring python module installer script to be (more) platform neut…
…ral and compatible with conventional make and CMake builds
@junghans
Copy link
Member

junghans left a comment

2 questions:

  • Does this support DESTDIR, e.g. make install-python DESTDIR=/some/packaging/path - rpm is using that.
  • How to pick a non-standard install location, e.g. on Fedora python packages for OpenMPI need to go into $MPI_PYTHON3_SITEARCH, which is something like /usr/lib64/python3.X/openmpi/.

In general, I don't like the idea of one buildsystem (CMake) calling another buildsystem (python-install.py).

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 22, 2019

2 questions:

  • Does this support DESTDIR, e.g. make install-python DESTDIR=/some/packaging/path - rpm is using that.

if you want to install into a custom location, you need to call python/install.py directly and set the -d flag

  • How to pick a non-standard install location, e.g. on Fedora python packages for OpenMPI need to go into $MPI_PYTHON3_SITEARCH, which is something like /usr/lib64/python3.X/openmpi/.

In general, I don't like the idea of one buildsystem (CMake) calling another buildsystem (python-install.py).

what has been there before was not working at all and incorrectly tied the LAMMPS python module to the PYTHON package. What this PR adds is a consistent behavior and default settings that work in both build systems for people that compile LAMMPS from source. This is the primary audience and not people packaging LAMMPS. Since packaging is scripted anyway, it should be easy to customize installation any which way you like in the packaging scripts.

using distutils is what i thought to be the "proper way" to install python modules. now both build systems do it.

@junghans

This comment has been minimized.

Copy link
Member

junghans commented Mar 22, 2019

Ok, I still don't like the fact that we are breaking with the standard CMake workflow, i.e. cmake ; make ; make install.

install-python
${PYTHON_EXECUTABLE} install.py -v ${LAMMPS_SOURCE_DIR}/version.h
-m ${CMAKE_CURRENT_SOURCE_DIR}/../python/lammps.py
-l ${CMAKE_BINARY_DIR}/liblammps${CMAKE_SHARED_LIBRARY_SUFFIX}

This comment has been minimized.

Copy link
@junghans

junghans Mar 22, 2019

Member

Could we add -d ${PYTHON_INSTDIR} here?

This comment has been minimized.

Copy link
@junghans

junghans Mar 26, 2019

Member

With the recent change, should we are -p here? CMake's default prefix already doesn't require root.

This comment has been minimized.

Copy link
@akohlmey

akohlmey Mar 26, 2019

Author Member

I am OK with any change, that keeps the conventional build and cmake builds with default settings consistent. E.g. on MacOSX the user site-packages folder for python is "$HOME/Library/Python/#.#/lib/python/site-packages". Does ${PYTHON_INSTDIR} by default point there? While on Linux it would have to point to "$HOME/.local/lib/python#.#/site-packages/" by default.

This comment has been minimized.

Copy link
@junghans

junghans Mar 26, 2019

Member

It runs import distutils.sysconfig as cg; print(cg.get_python_lib(1,0,prefix='${CMAKE_INSTALL_PREFIX})) to determine the library dir.

@junghans

This comment has been minimized.

Copy link
Member

junghans commented Mar 23, 2019

Please correct me if I am wrong, but the only thing install.py does, is to install liblammps.so in the site-package directory a second time and to create an egg file, correct?

Even though I have never seen there egg files uses for anything, we could easily create them from a template (like our pkg-config file) as the only part, which actually changes is the version string, e.g. something like:

Metadata-Version: 1.0
Name: lammps
Version: @LAMMPS_VERSION@
Summary: LAMMPS Molecular Dynamics Python module
Home-page: http://lammps.sandia.gov
Author: Steve Plimpton
Author-email: sjplimp@sandia.gov
License: UNKNOWN
Description: UNKNOWN
Platform: UNKNOWN
@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 25, 2019

@sjplimp do we need to keep the legacy installation scheme for the python module, that just copies the LAMMPS python module and the shared library to a given folder and then requires the user to explicitly set environment variables? I am thinking that if a user will set the environment variables, then they can point to the source (like you seem to prefer doing).

I am trying to make this consistent across platforms (Windows, MacOSX, and Linux all have different distribution trees and - sometimes - shared library extensions) and have this consistent across cmake and make build (so that something meaningful happens with "make install-python" in either case and whether it is called with superuser permissions or not) and calling the install script manually.

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 25, 2019

Ok, I still don't like the fact that we are breaking with the standard CMake workflow, i.e. cmake ; make ; make install.

this builds/installs LAMMPS, the executable.

LAMMPS, the python module is a different thing. We have plenty of people that only want the latter.

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 25, 2019

@junghans i've studied the distutils/setuptools docs, and there is a prefix option, which i have tried to expose to the install.py script. Would this be compatible with the installation tree requirements of Fedora?

What I want to bring to the CMake setup is, that make install-python will produce a working python module regardless of having root access or not. It will first try to install into the standard system tree, and if that fails (usually due to insufficient permissions), it will re-run the installation attempt with the --user flag added. After that the python module install should just work (well, at least on most Linux systems and MacOSX) without having to mess with environment variables.

akohlmey added some commits Mar 25, 2019

@junghans
Copy link
Member

junghans left a comment

I see what you want to do now, but I mainly want to avoid that a user set a prefix and the python module get nevertheless installed outside of it

Show resolved Hide resolved cmake/CMakeLists.txt
install-python
${PYTHON_EXECUTABLE} install.py -v ${LAMMPS_SOURCE_DIR}/version.h
-m ${CMAKE_CURRENT_SOURCE_DIR}/../python/lammps.py
-l ${CMAKE_BINARY_DIR}/liblammps${CMAKE_SHARED_LIBRARY_SUFFIX}

This comment has been minimized.

Copy link
@junghans

junghans Mar 26, 2019

Member

It runs import distutils.sysconfig as cg; print(cg.get_python_lib(1,0,prefix='${CMAKE_INSTALL_PREFIX})) to determine the library dir.

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 26, 2019

@junghans it looks like there are too many conflicts in how make install and make install-python is expected to work. however, earlier today i realized, that the common usage scenarios for those two targets and pretty orthogonal, i.e. a user that would want to do make install-python would not likely be interested in also doing make install and vice versa. So the resolution could be to install the python module with both targets, but with the different settings. For make install it makes sense to install the module when either the PYTHON package is enabled (as you can pass a reference to SELF which dlsym() will resolve properly even without a shared library, if LAMMPS has been linked to re-export all symbols, which is enabled by the linker flags of the python libraries) or the shared library is built. When neither is enabled, there is no way to initialize the python module, so make install will not copy lammps.py.

@junghans
Copy link
Member

junghans left a comment

This is a good compromise, as long as make install and its DESTDIR magic is working packaging will be easy and if user don't want to deal with CMAKE_INSTALL_PREFIX or PYTHON_INSTDIR can just run make install-python.

Some minor nitpicks.

WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/../python
COMMENT "Installing LAMMPS Python module"
)
if (${PYTHONINTERP_FOUND})

This comment has been minimized.

Copy link
@junghans

junghans Mar 26, 2019

Member

${} not needed.

else()
add_custom_target(
install-python
echo "Must have Python installed to install the LAMMPS Python module")

This comment has been minimized.

Copy link
@junghans

junghans Mar 26, 2019

Member

Nitpick: I would use ${CMAKE_COMMAND} -E echo for better Cross-Plattform support.

else()
add_custom_target(
install-python
echo "Must build LAMMPS as a shared library to use the Python module")

This comment has been minimized.

Copy link
@junghans

junghans Mar 26, 2019

Member

Same nitpick!

###############################################################################
if((BUILD_LIB AND BUILD_SHARED_LIBS) OR (PKG_PYTHON))
find_package(PythonInterp)
if (${PYTHONINTERP_FOUND})

This comment has been minimized.

Copy link
@junghans

junghans Mar 26, 2019

Member

Same for ${}.

if((BUILD_LIB AND BUILD_SHARED_LIBS) OR (PKG_PYTHON))
find_package(PythonInterp)
if (${PYTHONINTERP_FOUND})
if(NOT PYTHON_INSTDIR)

This comment has been minimized.

Copy link
@junghans

junghans Mar 26, 2019

Member

PYTHON_INSTDIR is not a real cached variable maybe it is time to make it one and have it default to the value below.

akohlmey added some commits Mar 27, 2019

@akohlmey akohlmey marked this pull request as ready for review Mar 27, 2019

@akohlmey akohlmey requested review from stanmoore1 and junghans Mar 27, 2019

suggested changes are implemented in 3867972

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Mar 27, 2019

@akohlmey I don’t have time to test this, but does this work with virtualenv?

@akohlmey

This comment has been minimized.

Copy link
Member Author

akohlmey commented Mar 27, 2019

@akohlmey akohlmey merged commit e63d2ce into lammps:master Mar 27, 2019

6 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/cmake/cmake-serial-pr head run ended
Details
lammps/pull-requests/kokkos-omp-pr head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details

@akohlmey akohlmey deleted the akohlmey:install-python-for-cmake branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.