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

Fix "autogen.pl --no-ompi" #1143

Merged
merged 1 commit into from
Dec 7, 2015
Merged

Fix "autogen.pl --no-ompi" #1143

merged 1 commit into from
Dec 7, 2015

Conversation

rhc54
Copy link
Contributor

@rhc54 rhc54 commented Nov 21, 2015

This was broken due to inclusion of a conditional (MCA_BUILD_ompi_pml_monitoring_DSO) in the test/monitoring Makefile.am that is only defined if OMPI is built.

@jsquyres @bosilca I suspect this isn't the right way to fix the build, but I've tried everything I can think of to resolve it with no luck. This seems to work, but looks ugly. Could you guys please look at this?

@ggouaillardet
Copy link
Contributor

my 0.02US$

all test/*/Makefile.am does define check_PROGRAMS except monitoring that defines noinst_PROGRAMS
check_PROGRAMS is never empty, even when there is no ompi
and I am not sure everything would go fine if that would be empty.

my best bet would be to update test/Makefile.am and append monitoring to SUBDIRS only if PROJECT_OMPI

I will not be able to test that before tuesday...

@@ -256,7 +256,9 @@ m4_ifdef([project_oshmem], [OSHMEM_CONFIGURE_OPTIONS])
# Set up project specific AM_CONDITIONALs
AS_IF([test "$enable_ompi" != "no"], [project_ompi_amc=true], [project_ompi_amc=false])
m4_ifndef([project_ompi], [project_ompi_amc=false])
AM_CONDITIONAL([PROJECT_OMPI], [test "$project_ompi_amc" = "true"])
AM_CONDITIONAL([PROJECT_OMPI], [test "$PROJECT_OMPI_amc" = "true"])
Copy link
Member

Choose a reason for hiding this comment

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

This change in case doesn't seem right -- the lower case version is defined on the three lines above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh - that is a typo. I didn't mean to change that line.

@jsquyres
Copy link
Member

A better solution might be to move the test/monitoring directory to be within ompi/mca/pml/monitoring. That way, it'll already naturally be traversed (or not), depending on whether the monitoring PML is going to be built or not.

@jsquyres
Copy link
Member

jsquyres commented Dec 6, 2015

@rhc54 @bosilca Thoughts on moving the test/monitoring directory to be in ompi/mca/pml/monitoring? That would make the whole problem much easier to solve.

@bosilca
Copy link
Member

bosilca commented Dec 6, 2015

We always put all our tests together in a single location. Scattering the tests all over the development tree is not only ugly but confusing.

@jsquyres
Copy link
Member

jsquyres commented Dec 6, 2015

It also makes the build conditionals a nightmare.

FWIW, the usnic BTL has had its tests in opal/mca/btl/usnic for quite a while. Similar with the debugger tests (in ompi/debuggers).

@bosilca
Copy link
Member

bosilca commented Dec 6, 2015

These are exceptions and not the norm. Moreover, the monitoring PML only depends on HWLOC which is now a required block in OMPI, and as a result the monitoring PML is supposedly always compiled. What is the problem with these tests?

@jsquyres
Copy link
Member

jsquyres commented Dec 6, 2015

The monitoring PML can be compiled out via --enable-mca-no-build (which I believe someone did).

@bosilca
Copy link
Member

bosilca commented Dec 6, 2015

Let me check more precisely what the problem is. The interaction between the PML an the rest of the world is done via MPIT pvars and cvars, and the test should have failed graciously.

@jsquyres
Copy link
Member

jsquyres commented Dec 6, 2015

I just closed up my laptop, but IIRC, it's a linking problem.

Sent from my phone. No type good.

@rhc54
Copy link
Contributor Author

rhc54 commented Dec 6, 2015

It isn't a linking problem - the issue is that MCA_BUILD_ompi_pml_monitoring_DSO is only defined if you are (a) building the OMPI layer and (b) didn't exclude that component. So autogen fails because that parameter hasn't been defined and yet is used in the test Makefile.am.

@bosilca
Copy link
Member

bosilca commented Dec 7, 2015

How do we protect the MPI tests today ?

@rhc54
Copy link
Contributor Author

rhc54 commented Dec 7, 2015

The problem is simple:

if PROJECT_OMPI
    noinst_PROGRAMS = monitoring_test
    monitoring_test_SOURCES = monitoring_test.c
    monitoring_test_LDFLAGS = $(WRAPPER_EXTRA_LDFLAGS)
    monitoring_test_LDADD = $(top_builddir)/ompi/libmpi.la $(top_builddir)/opal/libopen-pal.la

if MCA_BUILD_ompi_pml_monitoring_DSO
    lib_LTLIBRARIES = monitoring_prof.la
    monitoring_prof_la_SOURCES = monitoring_prof.c
    monitoring_prof_la_LDFLAGS=-module -avoid-version -shared $(WRAPPER_EXTRA_LDFLAGS)
    monitoring_prof_la_LIBADD = $(top_builddir)/ompi/libmpi.la $(top_builddir)/opal/libopen-pal.la
endif

endif

That second if statement is -not- protected by the first one - autogen still requires that the param being tested must be defined, even if the first if is not "true"

@ggouaillardet
Copy link
Contributor

fwiw an other way to fix this is to update test/Makefile.am and append monitoring to SUBDIRS only if PROJECT_OMPI

that might be a bit convoluted ...

imho, i agree with @bosilca we should keep the tests in more or less the same location
what we could do is have several directories in test

test/opal
test/orte
test/ompi

the pros is that would fix such kind of issues in an elegant way
the cons is we would have to split the datatype directory (for example) into opal/datatype and ompi/datatype

@rhc54
Copy link
Contributor Author

rhc54 commented Dec 7, 2015

@ggouaillardet I tried your suggestion, and it does not work - autogen still insists that the parameter must be defined:

opal/Makefile.am: installing 'config/depcomp'
test/monitoring/Makefile.am:23: error: MCA_BUILD_ompi_pml_monitoring_DSO does not appear in AM_CONDITIONAL
autoreconf: /opt/local/bin/automake failed with exit status: 1

@bosilca
Copy link
Member

bosilca commented Dec 7, 2015

Let's comment out the generation of the shared library. This library is a PMPI type of interception layer that can be linked with any applications in order to get automatic support for the monitoring framework. It is not necessary for the testing.

@ggouaillardet
Copy link
Contributor

@rhc54 oops, my bad
here is the full (and tested) patch

diff --git a/configure.ac b/configure.ac
index eec43fc..3e23a3c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1377,8 +1377,9 @@ AC_CONFIG_FILES([
     test/support/Makefile
     test/threads/Makefile
     test/util/Makefile
-    test/monitoring/Makefile
 ])
+m4_ifdef([project_ompi], [AC_CONFIG_FILES([test/monitoring/Makefile])])
+
 AC_CONFIG_FILES([contrib/dist/mofed/debian/rules],
                 [chmod +x contrib/dist/mofed/debian/rules])
 AC_CONFIG_FILES([contrib/dist/mofed/compile_debian_mlnx_example],
diff --git a/test/Makefile.am b/test/Makefile.am
index 5252fd5..5d5323d 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -11,6 +11,8 @@
 #                         All rights reserved.
 # Copyright (c) 2009      Sun Microsystems, Inc. All rights reserved.
 # Copyright (c) 2010      Cisco Systems, Inc. All rights reserved.
+# Copyright (c) 2015      Research Organization for Information Science
+#                         and Technology (RIST). All rights reserved.
 # $COPYRIGHT$
 #
 # Additional copyrights may follow
@@ -19,5 +21,8 @@
 #

 # support needs to be first for dependencies
-SUBDIRS = support asm class threads datatype util monitoring
+SUBDIRS = support asm class threads datatype util
+if PROJECT_OMPI
+SUBDIRS += monitoring
+endif
 DIST_SUBDIRS = event $(SUBDIRS)

…nditional in the test/monitoring Makefile.am that is only defined if OMPI is built.

Per suggestion from @bosilca, comment out generation of the shared library

Use the patch from Gilles instead
@rhc54
Copy link
Contributor Author

rhc54 commented Dec 7, 2015

@ggouaillardet Looks fine to me, and it seems to work, so let's go with that approach. Thx!

rhc54 pushed a commit that referenced this pull request Dec 7, 2015
Fix "autogen.pl --no-ompi"
@rhc54 rhc54 merged commit 04fc406 into open-mpi:master Dec 7, 2015
@rhc54 rhc54 deleted the topic/noompi branch December 7, 2015 08:40
@jsquyres
Copy link
Member

jsquyres commented Dec 7, 2015

@rhc54 @ggouaillardet @bosilca No no no! This does not solve the problem of conditional compilation.

If I configure --enable-mca-no-build=pml-monitoring, you will still have a linking problem.

The tests for the monitoring PML need to only be built when the monitoring PML itself is built. The best way to to do that is to put the tests in the same directory as the monitoring PML itself, so that if the monitoring PML is not built, its directory is skipped and the tests aren't built, either.

The whole GNU make check architecture is designed to allow tests to be anywhere in the tree. It's common and useful to locate tests with the code that they are testing, not randomly somewhere else in the tree. Yes, we have tests in tests, but that's somewhat of a historical artifact -- it's not necessarily the only way that we should do things.

jsquyres pushed a commit to jsquyres/ompi that referenced this pull request Aug 23, 2016
…_ireduce_scatter_block

coll/libnbc: fix MPI_Ireduce_scatter_block for one task communicator
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.

4 participants