Skip to content

Working Library Build and Installation#6

Open
AlexBuccheri wants to merge 31 commits into
masterfrom
fix_proposed_structure
Open

Working Library Build and Installation#6
AlexBuccheri wants to merge 31 commits into
masterfrom
fix_proposed_structure

Conversation

@AlexBuccheri
Copy link
Copy Markdown
Owner

@AlexBuccheri AlexBuccheri commented Feb 9, 2021

Code is ready to be reviewed. We'll get the tests running on the next PR.
Also note that MPI08=off does not work. We need to write setters for the class.

Max Williams and others added 7 commits February 8, 2021 17:54
* Add src/errors_warnings/assert.F90 to CMakeLists.txt
* Correct MPI_comm to MPI_Comm to match MPI casing of types. 
* Remove indentation of #ifdef directives
* TODO: work out why MPI_COMM_DUP() call is coming up as missing a specific subroutine for the passed arguments and argument types.
…sing variables. -DMPI08=On should be the default (set in CMake if no argument given). Still looks like there's an issue with correct use of release and debug flags.
Comment thread cmake/PreprocessorSettings.cmake Outdated
if(CMAKE_BUILD_TYPE MATCHES Debug)
# Preprocessor variable "DEBUG" in the code
set_property(TARGET ${TARGET} APPEND PROPERTY
COMPILE_DEFINITIONS "DEBUG")
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Probs change back to USE_ASSERT. More descriptive

Comment thread src/errors_warnings/asserts.F90 Outdated
subroutine assert_true(logical_condition, message)
!> Condition to test
logical, intent(in) :: logical_condition
#ifdef DEBUG
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Probs change back to USE_ASSRT

@AlexBuccheri AlexBuccheri changed the title WIP: Clean up Working Library Build and Installation Feb 20, 2021
Copy link
Copy Markdown
Owner Author

@AlexBuccheri AlexBuccheri left a comment

Choose a reason for hiding this comment

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

I'm broadly happy with this. Tested and it all compiles. CMake will stop the old MPI from being used for the time-being. Will add the testing on the next MR: Need to try manually linking to test code before using the test framework and automating test builds with CMake

Comment thread src/tests/test_mpilib20_init.f90 Outdated
@@ -0,0 +1,19 @@
!> Driver program to test mpilib20_init_finalise.f90
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not tested yet but also not used in the build. Will get on next MR

Comment thread src/tests/test_mpilib20_init_thread.f90 Outdated
@@ -0,0 +1,20 @@
!> Driver program to test mpilib20_init_finalise.f90
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not tested yet but also not used in the build. Will get on next MR

@@ -0,0 +1,53 @@
!> Test initialisation and finalisation of the MPI environment
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not tested yet but also not used in the build. Will get on next MR

Comment thread src/routines/mpilib20_init_finalise.f90 Outdated
public :: mpilib20_init, mpilib20_init_thread, mpilib20_finalize

contains

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Implemented but not yet used

Comment thread src/routines/mpilib20_init_finalise.f90 Outdated
procedure, public :: finalize => finalise_mpi_env !! Terminate instance of MPI environment object
procedure, public :: get_comm => get_communicator !! Get the communicator
! Private routines
!procedure :: set_comm !! Set the communicator... may not need
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not sure if private data can be accessed within the module in which the type is defined. Would assume not but then this is fortran.

Comment thread src/routines/mpilib20_init_finalise.f90 Outdated
module mpilib20_init_finalise
use, intrinsic :: iso_fortran_env, only: error_unit
use mpi_bindings, only: MPI_comm, MPI_Group
use mpi_bindings, only : MPI_Comm, MPI_Group, MPI_INIT, MPI_COMM_DUP, &
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Moved them up here - thought it looked a bit cleaner

@@ -0,0 +1,204 @@
#!/usr/bin/python3
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

We may not use this. Either way, not worth reviewing this file. Copied and pasted from a different project

Comment thread cmake/unit_test_functions.cmake
…up to use zofus mpi object (hence the test report returns for every process with no process id. One could assert input and output on master only, but would be better to have more flexibility. Move to setup and teardowns
…ur test modules. Same error - wondering if I built zofu with a different MPI version or flavour
Copy link
Copy Markdown
Collaborator

@maxwilliams94 maxwilliams94 left a comment

Choose a reason for hiding this comment

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

Nice work on the setters and getters. Agree that private routines separate from the public api can be cut down.

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.

2 participants