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

Detect devel build in vpath builds #12173

Closed
wants to merge 1 commit into from

Conversation

devreal
Copy link
Contributor

@devreal devreal commented Dec 19, 2023

Use git describe on a known OMPI commit sha instead of checking for .git directory. Git will walk up the tree trying to find the git metadata. This enables devel builds in typical vpath builds:

$ mkdir build
$ cd build
$ ../configure ...

@rhc54
Copy link
Contributor

rhc54 commented Dec 19, 2023

Very nice - I "stole" it for PMIx and PRRTE 😄 Thanks!

rhc54 added a commit to rhc54/openpmix that referenced this pull request Dec 19, 2023
…git directory

Supports VPATH builds. Copied from open-mpi/ompi#12173.

Thanks to @devreal for the original PR.

Signed-off-by: Ralph Castain <[email protected]>
rhc54 added a commit to rhc54/prrte that referenced this pull request Dec 19, 2023
….git directory

Supports VPATH builds. Copied from open-mpi/ompi#12173.

Thanks to @devreal for the original PR.

Signed-off-by: Ralph Castain <[email protected]>
rhc54 added a commit to openpmix/openpmix that referenced this pull request Dec 19, 2023
…git directory

Supports VPATH builds. Copied from open-mpi/ompi#12173.

Thanks to @devreal for the original PR.

Signed-off-by: Ralph Castain <[email protected]>
rhc54 added a commit to openpmix/prrte that referenced this pull request Dec 19, 2023
….git directory

Supports VPATH builds. Copied from open-mpi/ompi#12173.

Thanks to @devreal for the original PR.

Signed-off-by: Ralph Castain <[email protected]>
Use `git describe` on a known OMPI commit sha instead
of checking for .git directory.

Signed-off-by: Joseph Schuchart <[email protected]>
@devreal
Copy link
Contributor Author

devreal commented Dec 20, 2023

I'm worried that this may break if there is no git command available on a system. What is the right way to detect whether git is available in m4?

@rhc54
Copy link
Contributor

rhc54 commented Dec 20, 2023

Good point - here is how we check for cython:

        AC_CHECK_PROG(pmix_cython_rpm, cython, [cython])
        if test "$pmix_cython_rpm" != ""; then
            AC_MSG_CHECKING([Cython version])
            cyvers=`cython --version 2>&1`
            cython_version=${cyvers#"Cython version "}
            AC_MSG_RESULT([$cython_version])
            PMIX_SUMMARY_ADD([Bindings], [Cython], [], [yes ($cython_version)])
        else
            AC_MSG_WARN([Python bindings were enabled, but the Cython])
            AC_MSG_WARN([package was not found. PMIx Python bindings])
            AC_MSG_WARN([require that the Cython package be installed])
            AC_MSG_ERROR([Cannot continue])
        fi

So for this case, you'd want something like:

        AC_CHECK_PROG(pmix_git_cmd, git, [git])
        if test "$pmix_git_cmd" != ""; then
            ...do your test for git repo
        fi

@rhc54
Copy link
Contributor

rhc54 commented Dec 20, 2023

FWIW, that works just fine.

#

if test -d .git; then
if git describe 43a3f4282055c7116ca618c17a9f27247f4923d2 &> /dev/null ; then
Copy link
Member

Choose a reason for hiding this comment

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

I had to think about this a bit; I had a knee-jerk "eww!" reaction to this, but I can't quite quantify why. I.e., it feels weird to do this, but I can't find any flaw in the reasoning behind it.

Can we make a few minor changes, though?

  1. The code @rhc54 suggested looking for the git command seems reasonable; we don't want an error to occur here if there's no git command available.
  2. Can you put in a comment explaining why we're doing this? Some points to make, in no particular order:
    • We want to know if we're in a Git tree, and git describe will walk backwards up the tree to catch the case of VPATH builds in a subdirectory of a git clone
    • We know that this won't catch VPATH builds outside of a git clone. It's not meant to be a perfect solution.
    • The intent of checking for a specific commit is to ensure that we're in an OMPI git clone (not just any arbitrary git clone) -- e.g., if someone expands an OMPI tarball in a git clone of some other repo (could happen in Homebrew, Spack, or EasyBuild, or some other build-scripts kind of git repo), we wouldn't want that tarball build to default to OMPI-developer values.
    • Instead of checking for 43a3f42 (which is a pretty recent commit), can you check for 350564b? That's the root commit of OMPI's main. I realize this won't make much of a functional difference compared to checking for 43a3f42, but it's a bit more of a definitive location to check for, and easy to describe ("we're looking for the known root commit of Open MPI's main repo").

@devreal
Copy link
Contributor Author

devreal commented Dec 20, 2023

Thanks for the feedback @rhc54 and @jsquyres! Maybe the better solution is to add $OPAL_TOP_SRCDIR to the .git/ check (like PMIX already does)? That way we do support full out-of-tree builds and don't have to rely on git to be available and a (seemingly) random git sha...

So the test would be:

if test -d ${OPAL_TOP_SRCDIR}/.git; then
    OPAL_DEVEL=1
else
    OPAL_DEVEL=0
fi

@jsquyres
Copy link
Member

That certainly is a lot simpler, and covers more cases.

@rhc54
Copy link
Contributor

rhc54 commented Dec 20, 2023

That is what I have historically done and seems to work okay. Only issue I can see is that it relies on TOP_SRCDIR to be correctly set for your code tree - which it should be (even when embedded in someone else's code) since your configure code sets it based on where it sees the OMPI source.

@devreal
Copy link
Contributor Author

devreal commented Jan 9, 2024

Closing, superseded by #12215

@devreal devreal closed this Jan 9, 2024
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