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

CMake minor fixes #3258

Merged
merged 6 commits into from
Oct 18, 2019
Merged

CMake minor fixes #3258

merged 6 commits into from
Oct 18, 2019

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Oct 17, 2019

Description of changes:

  • change next milestone to 4.2
  • load GNUInstallDirs to make standard GNU paths accessible from CMake variables
  • simplify CMake logic and install in python3.X folder instead of the deprecated python3 folder
  • add extra check to make sure install paths are correctly configured (all python and shared object files must be inside the package espressomd)

@jngrad jngrad added the CMake label Oct 17, 2019
@jngrad jngrad added this to the Espresso 4.1.1 milestone Oct 17, 2019
@codecov
Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #3258 into python will decrease coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3258   +/-   ##
======================================
- Coverage      85%     85%   -1%     
======================================
  Files         531     531           
  Lines       25796   25796           
======================================
- Hits        22169   22168    -1     
- Misses       3627    3628    +1
Impacted Files Coverage Δ
src/core/particle_data.cpp 96% <0%> (-1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34c6bd7...db8cb80. Read the comment docs.

@@ -20,6 +20,7 @@

cmake_minimum_required(VERSION 3.4)
include(FeatureSummary)
include(GNUInstallDirs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the block

if(NOT DEFINED BINDIR)
  set(BINDIR "bin")
endif(NOT DEFINED BINDIR)

in line 401 should be removed now. Did you check that the paths provided by GNUInstallDirs are used for all install targets? I did a quick grep and did find nothing, but I only checked the install commands, not sure
if this effects rpaths and so on....

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I overlooked that one. The install() commands have to be configured such that submodules get installed in the python espressomd folder, if not the new test in c43d057 will detect that (it searches for misplaced files in CMAKE_INSTALL_PREFIX). The GNU variables inherit from CMAKE_INSTALL_PREFIX, so we should use them as often as possible.

@jngrad
Copy link
Member Author

jngrad commented Oct 17, 2019

I've a few more improvements.

The Python guard isn't necessary anymore because submodules are
now installed in the Python package espressomd, and the testsuite
itself already has a Python guard in the top-level CMakeLists.txt.
@jngrad
Copy link
Member Author

jngrad commented Oct 17, 2019

The failures in CI are actually a bug in the build system, not a regression. Working on it.

Remove the non-standard CMAKE_INSTALL_EXEC_PREFIX variable in
favor of CMAKE_INSTALL_PREFIX. Install espressomd in the standard
python3.X folder instead of the deprecated python3 folder. Remove
duplicated code logic. Use explicit keyword arguments in calls to
functions from distutils.sysconfig.
@@ -241,7 +242,7 @@ if(WITH_PYTHON)
execute_process(
COMMAND
${PYTHON_EXECUTABLE} -c
"import distutils.sysconfig as cg; print(cg.get_python_lib(1,0,prefix='${CMAKE_INSTALL_EXEC_PREFIX}'))"
"import distutils.sysconfig as cg; print(cg.get_python_lib(prefix='${CMAKE_INSTALL_PREFIX}', plat_specific=True, standard_lib=False).replace('${CMAKE_INSTALL_PREFIX}/', '', 1))"
Copy link
Member Author

Choose a reason for hiding this comment

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

@junghans this change might affect your packaging workflow

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for asking, but it won't as PYTHON_INSTDIR is set for packaging, so this call isn't actually used.

@jngrad
Copy link
Member Author

jngrad commented Oct 18, 2019

bors r=fweik

bors bot added a commit that referenced this pull request Oct 18, 2019
3216: Refactor ghosts.cpp r=KaiSzuttor a=hirschsn

More refactoring that builds upon #3212 

Description of *major* changes:
 - remove GhostCommunication::mpi_comm as it is not used in ghosts.cpp,
 - make GhostCommunication::part_lists a std::vector,
 - remove static variables s_buffer and r_buffer,
 - factor out memory handling,
 - change loops to range based for,
 - use boost::mpi.
 - Replace the manual poststore and prefetch loops by find_ifs

Mainly, ghosts.cpp now defines CommBuf, which is a container for the data to be sent or received as well as two classes (Archiver and BondArchiver), that insert and extract the memory from CommBuf.

Some of these changes, like the removal of static variables, is necessary for my implementation of asynchronous ghost communication.

PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [ ] Docs?


3239: Added test criteria for the charged_system-2 tutorial r=RudolfWeeber,jngrad a=reinaual



3253: Refactor NpT public interface r=fweik a=jngrad

Description of changes:

- remove the silent conversion of the incorrect input parameter `dimension=[0,0,0]` to `[1,1,1]` in the core (bypassing sanity checks), now the checks will throw an exception for fixed-volume NpT; the original behavior was counter-intuitive and undocumented until 2 days ago
- remove the automatic decay of NpT to NVT upon initialization of NpT with incorrect parameters
- remove unused `p_inst_av` variable (average instantaneous pressure)
- cleanup integrator documentation


3258: CMake minor fixes r=fweik a=jngrad

Description of changes:
- change next milestone to 4.2
- load `GNUInstallDirs` to make standard GNU paths accessible from CMake variables
- simplify CMake logic and install in `python3.X` folder instead of the deprecated `python3` folder
- add extra check to make sure install paths are correctly configured (all python and shared object files must be inside the package `espressomd`)


Co-authored-by: Steffen Hirschmann <steffen.hirschmann@ipvs.uni-stuttgart.de>
Co-authored-by: Florian Weik <fweik@icp.uni-stuttgart.de>
Co-authored-by: Alexander Reinauer <st144434@stud.uni-stuttgart.de>
Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
@bors
Copy link
Contributor

bors bot commented Oct 18, 2019

Build succeeded

@bors bors bot merged commit db8cb80 into espressomd:python Oct 18, 2019
@jngrad jngrad deleted the cmake-update branch January 18, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants