Skip to content

Conversation

@teojgo
Copy link
Contributor

@teojgo teojgo commented Aug 21, 2018

  • Adapt to build systems.

  • Adapt to new syntax.

  • Remove unused Makefiles

  • Add build dependencies in the F90 Makefile in order to
    make the Fortran modules available.

Closes #368
Addresses #397

* Adapt to build systems.

* Adapt to new syntax.

* Remove unused Makefiles

* Add build dependencies in the F90 Makefile in order to
  make the Fortran modules available.
* Revert removal of `Makefile`.

* Remove `Makefile_ddt` from `src` and all programming languages.

* Restrict `max_concurrency` in case of Fortran.
@codecov-io
Copy link

codecov-io commented Aug 22, 2018

Codecov Report

Merging #425 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #425      +/-   ##
=========================================
- Coverage   91.21%   91.2%   -0.02%     
=========================================
  Files          70      70              
  Lines        8585    8585              
=========================================
- Hits         7831    7830       -1     
- Misses        754     755       +1
Impacted Files Coverage Δ
unittests/test_policies.py 99.09% <0%> (-0.46%) ⬇️

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 41595e7...36f3fd2. Read the comment docs.

Copy link
Contributor

@kraushm kraushm left a comment

Choose a reason for hiding this comment

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

lgtm

self.extension = extension
self.makefile = 'Makefile'
self.build_system = 'Make'
self.build_system.makefile = 'Makefile'
Copy link
Contributor

Choose a reason for hiding this comment

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

Makefile is the default. Why do you set it?

# 'PrgEnv-pgi': ' -O2 -mp'
}
self.flags = ' -g'
self.flags = ['-g']
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you put directly the -g flag inside the self.prgenv_flags. I think the code would be more readable.

self.ddt_options = []
self.keep_files = ['ddtreport.txt']

def _set_compiler_flags(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this function?

]

self.flags += ['-DUSE_MPI']
self.flags += ['-D_CSCS_ITMAX=5']
Copy link
Contributor

Choose a reason for hiding this comment

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

If you set self.build_systems.cppflags, the trick with the flags attribute won't be needed.


if self.current_system.name == 'kesch':
arch = 'sm_37'
self.flags += ['-lm', '-lcudart']
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here, you should set the ldflags of the build system.


self.flags += ['-DUSE_MPI']
self.flags += ['-D_CSCS_ITMAX=5']
self.build_system.cflags = ['-DUSE_MPI', '-D_CSCS_ITMAX=5']
Copy link
Contributor

Choose a reason for hiding this comment

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

These are cppflags (i.e., preprocessor flags). Here, you will only set the CFLAGS in the Makefile invocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the Makefile has to use them during compilation right?

@vkarak
Copy link
Contributor

vkarak commented Aug 23, 2018

@teojgo There seems to be a bug in the test for Kesch.

@vkarak vkarak merged commit 9dab152 into reframe-hpc:master Aug 24, 2018
@teojgo teojgo deleted the regression_test/ddt_pe18.07 branch August 31, 2018 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants