Skip to content

add support to enable integration of pscom in psmpi easyblock#1397

Merged
boegel merged 3 commits intoeasybuilders:developfrom
damianam:psmpi_pscom_allin
May 22, 2018
Merged

add support to enable integration of pscom in psmpi easyblock#1397
boegel merged 3 commits intoeasybuilders:developfrom
damianam:psmpi_pscom_allin

Conversation

@damianam
Copy link
Copy Markdown
Member

This enables integration of pscom in psmpi build

Comment thread easybuild/easyblocks/p/psmpi.py Outdated
pscom_allin_path = self.cfg['pscom_allin_path'].strip()
self.cfg.update('configopts', ' --with-pscom-allin="%s"' % pscom_allin_path)
self.cfg.update('preconfigopts', ('PSCOM_LDFLAGS=-L{0}/lib '+
'PSCOM_CPPFLAGS=-I{0}/include').format(pscom_allin_path))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would reformat this a bit for readability, and to avoid duplicate code:

if self.cfg['pscom_allin_path'] is None:
    pscom_allin_path = get_software_root('pscom')
else:
    pscom_allin_path = self.cfg['pscom_allin_path'].strip()
    self.cfg.update('configopts', ' --with-pscom-allin="%s"' % pscom_allin_path)

pscom_flags = "PSCOM_LDFLAGS=-L{0}/lib PSCOM_CPPFLAGS=-I{0}/include".format(pscom_allin_path)
self.cfg.update('preconfigopts', pscom_flags)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in the next commit

Comment thread easybuild/easyblocks/p/psmpi.py Outdated
self.cfg.update('preconfigopts', ('PSCOM_LDFLAGS=-L{0}/lib '+
'PSCOM_CPPFLAGS=-I{0}/include').format(get_software_root('pscom')))
else:
pscom_allin_path = self.cfg['pscom_allin_path'].strip()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do you need the .strip() here, that's a bit strange?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Otherwise there was an empty space that messed up the definition of the variable

extra_vars.update({
'mpich_opts': [None, "Optional options to configure MPICH", CUSTOM],
'threaded': [False, "Enable multithreaded build (which is slower)", CUSTOM],
'pscom_allin_path': [None, "Enable pscom integration by giving its source path", CUSTOM],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/source path/installation prefix/ ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, it is an installation prefix just if it is installed, right? But with the allin option it is not really installed so....... I still think source path is "correcter"

@boegel boegel changed the title Psmpi pscom allin add support to enable integration of pscom in psmpi easyblock Apr 12, 2018
@ocaisa
Copy link
Copy Markdown
Member

ocaisa commented Apr 12, 2018

I'm not sure I fully understand what this means? Is it that pscom becomes a build dependency only? In the past we've been able to switch out pscom for a new version, but this change would prevent that so what is the advantage?

@damianam
Copy link
Copy Markdown
Member Author

@ocaisa with this change pscom is integrated directly in psmpi. It is not a dependency of any kind. The advantage is enabling the compiler to use IPO, and to avoid the use of procedure lookup tables when calling library functions. This has a big impact on KNL, and that's the whole reason behind this change. In other processors it doesn't really make sense.

@boegel boegel added this to the 3.6.1 milestone May 22, 2018
@boegel boegel merged commit 03fd0d4 into easybuilders:develop May 22, 2018
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.

3 participants