Fix misc issues evidenced whey building on msys2 arch

[ggouaillardet]

No description provided.

[ggouaillardet]

@jsquyres could you please review this ?

the third commit open-mip/ompi@0289040309efec51628d7ab0e4ebb7cbb8753f28 fixes parallel build (configure --disable-mca-dso && make -j on www.msys2.org arch, not sure the arch is involved though). Serial make always works, but I added missing dependency (create the symlink after the linked file is built) in order to make make -j a happy panda. I revamped the logic (e.g. replace if tests at build time with a if at configure time), and I would like to you review this since I might have missed something.

[ibm-ompi]

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/f083c52b6188d8cfcff4183f5d490f03

[jsquyres]

A few ancillary notes:

1. I note that $(OMPI_V_LN_S) is in $topdir/Makefile.ompi-rules. We should probably just be including that file in these .am files rather than re-defining those macros (I'm guessing this just happened organically over time -- i.e., they were probably first defined in some common/foo/Makefile.am and then got copied to other common/*/Makefile.ams).
2. These Makefile.ams (and possibly/probably others?) have fragmented comments describing the .la creation scheme (e.g., some say "... case 2, described above" -- but it's not described above).
   • Is opal/mca/common/sm/Makefile.am the origin of all of these comments? If so, it might be good to just refer to that sm Makefile.am in all the others rather than having fragmented / incoherent comments.

Per this PR:

1. Did you have to move the all-local and clean-local targets up inside the AM if because of the new dependency on all-local? Otherwise, I'm not sure why you moved them up inside the if. I ask because I have a super-dim recollection that there may be problems with conditionally defining targets...?

[ggouaillardet]

@jsquyres I made some of the requested changes.

It seems I do not have to move the all-local and clean-local targets after all.
That being said, unless there is a good reason not to do so, I'd rather do it anyway for aesthetic reasons
(why generate a test that will be performed at make time when we already know the outcome at configure time)

[ggouaillardet]

note the last commit is not meant to be committed.

[ggouaillardet]

:bot:aws:retest

[jsquyres]

Looks much better, thanks.

Minor quibble: please change the comment from:

# See the comments in opal/mca/common/sm/Makefile.am for the explanations

to (re-flow this as appropriate):

# Common MCA components have complicated requirements and build sequences.
# See the comments in opal/mca/common/sm/Makefile.am for an explanation of
# requirements and building.

Also, please put a comment above the all-local: $(comp_noinst) referring back to sm/Makefile.am (because this is a confusing issue -- it's worth calling out explicitly when people copy-paste to new common Makefile.ams):

# Note that `all-local` needs to depend on $(comp_noinst).
# See opal/mca/common/sm/Makefile.am for an explanation
all-local: $(comp_noinst)

And in opal/mca/common/sm/Makefile.am, add a comment like this before the all-local rule (re-flow it as appropriate):

# all-local needs to depend on $(comp_noinst) to ensure proper ordering in parallel builds.
# Specifically, we need to ensure that the real library $(comp_noist) 
# is created before all-local tries to sym link to it.

[jsquyres]

Since we're now including Makefile.ompi-rules, I think you need to also change OMPI_V_LN_SCOMP to OMPI_V_LN_S everywhere.

[jsquyres]

The commit message on this commit no longer matches what the commit actually does.

I don't know / care if you want to split this into multiple commits:

• Add the dependencies
• Fix the comments
• Use Makefile.ompi-rules

[jsquyres]

@ggouaillardet I added the DNM label so that you don't accidentally merge with that last commit ("note the last commit is not meant to be committed").

[ggouaillardet]

@jsquyres thanks for the review, I will make the requested changes.

I just noted that since $(comp_noinst) is now a dependency of all-local, that means we are always building $(comp_noinst) and for some reasons, make clean fails to remove it when it is useless.

Unless we choose to make all-local a conditional target, I will unconditionally build noinst_LTLIBRARIES so make clean can properly and automatically remove it and hence fix make distclean.

[ibm-ompi]

The IBM CI (GNU/Scale) build failed! Please review the log, linked below.

Gist: https://gist.github.com/8226fa70da089f68c8f204ce17635812

[lanl-ompi]

Can one of the admins verify this patch?

[jsquyres]

@ggouaillardet Do you plan to finish this PR?

[ibm-ompi]

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/6d27c9e1e19019490cb6acaffd8162f4

[hppritcha]

Closing. Please reopen if needed.