Skip to content

Conversation

@ggouaillardet
Copy link
Contributor

  • move the mpi-io configury option into config/ompi_configure_options.m4
  • add ompi/mca/common/configure.m4 so this module is not built when
    Open MPI is configure'd with --disable-mpi-io

Fixes #2009

@ggouaillardet
Copy link
Contributor Author

@jsquyres this is a variant of #2010
can you please advise or merge the most suitable PR ?

@edgargabriel FYI

# -------------------------------------------
AC_DEFUN([MCA_ompi_common_CONFIG],
[
MCA_CONFIGURE_FRAMEWORK([$1], [$2], [$define_mpi_io])
Copy link
Member

Choose a reason for hiding this comment

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

Clever! 👍

Copy link
Member

Choose a reason for hiding this comment

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

...actually, after my first "oh, that's cool!" reaction, I don't think that this is quite right yet.

This solution will disable all common components when --disable-mpi-io is used, even if those common components have nothing to do with MPI IO.

Granted, right now, there's only ompi/mca/common/ompio, but there could well be other common components that are unrelated to MPI IO (we have a bunch in opal/mca/common, for example).

Can you adjust?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, will do

@ggouaillardet
Copy link
Contributor Author

@jsquyres i think i got it right this time, can you please double check ?

note you can also directly close this PR and merge #2020 (--disable-io-ompio) since it includes this commit

@jsquyres
Copy link
Member

I'm inclined to merge this one and not merge #2020 -- do we really need --disable-io-ompi? Come to think of it, why do we (still) have --disable-io-romio? Both of these components (and hopefully common/ompio and any other ompi-related components...?) can be disabled via --enable-mca-no-build. Do we need top-level switches for ROMIO / OMPIO?

The use case for disabling MPI IO altogether (including the PMI/MPI IO API functions) is different: it allows Open MPI with an external installation of ROMIO.

@jjhursey
Copy link
Member

The reason for having a --disable-io-ompi is because it's not easy to enumerate all of the frameworks (/components) that are related to OMPIO (I had a hard time making sure I got them all). For ROMIO it is pretty straightforward. That's why I would advocate for having a --disable-io-ompi option to pair with the --disable-io-romio. Even if that just turns around and enumerates the --enable-mca-no-build (Which seems like a fine way to go about it as long as it is maintained).

@jsquyres jsquyres changed the title configury: fix --disable-mpi-io configury: fix --disable-mpi-io for static builds Sep 21, 2016
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

@ggouaillardet I think this is now nearly correct, and we should probably close #2010 in favor of this one.

@jjhursey Ok, I see your point on --disable-io-ompio -- we should probably treat #2020 as a separate issue, and finish that one once this one is merged.

[Disable built-in support for MPI-2 I/O, likely because
an externally-provided MPI I/O package will be used.
Default is to use the internal component system and
its specially modified version of ROMIO])])
Copy link
Member

@jsquyres jsquyres Sep 21, 2016

Choose a reason for hiding this comment

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

The default is now to use OMPIO, not ROMIO, right?

I know you just copied this help text, but I think it is now stale / outdated and needed to be updated anyway... 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will revamp that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this context, i think we want to say ompi/mca/io framework vs external MPI-IO library

AC_DEFUN([MCA_ompi_common_ompio_CONFIG],[
AC_CONFIG_FILES([ompi/mca/common/ompio/Makefile])

AS_IF([test "$define_mpi_io" = "1"],
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to put a comment here explaining what $define_mpi_io is, and why it can enable/disable the entire common/ompi framework (I know you just copied the code from before, but that name is a bit... weird, and doesn't really follow the conventions of other enable/disable vars elsewhere in the code base).


OMPI_MPIF_IO_CONSTANTS_INCLUDE=
OMPI_MPIF_IO_HANDLES_INCLUDE=
AS_IF([test "$enable_mpi_io" != "no"],
Copy link
Member

Choose a reason for hiding this comment

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

Should this line now be checking $define_mpi_io instead of $enable_mpi_io?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is equivalent, but let's change it to be more consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on second thought, i'd rather use $enable_mpi_io everywhere, and locally define $define_mpi_io only where needed in ompi/mca/io/configure.m4

 - move the mpi-io configury option into config/ompi_configure_options.m4
 - add ompi/mca/common/ompio/configure.m4 so this component is not built when
   Open MPI is configure'd with --disable-mpi-io

Fixes open-mpi#2009
@ggouaillardet
Copy link
Contributor Author

@jsquyres i made the changes

@ggouaillardet ggouaillardet merged commit 505be0e into open-mpi:master Sep 21, 2016
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