configure: add check for valid version

[wihobbs]

Problem: autoconf will accept junk at configure time as a valid version, then go off and generate an invalid version.h file. This happens frequently with shallow clones, setups in CI, or other constrained user environments. It has caused new contributors a lot of confusion in the past.

Solution: Like flux-framework/flux-sched#1291 suggested, reject invalid versions at configure time and provide appropriate suggestions to the user to remedy this.

[wihobbs]

"Shell scripts make shell scripts by the C shore" If autoconf is written in C, that should be their new motto

update: autoconf is not written in C

[garlick]

This is a good idea.

Well couple of suggestions.

• autoconf has macros for performing checks and emitting output so we should probably use AC_MSG_CHECKING() and AC_MSG_RESULT().
• there is probably a more compact way to check that the numbers are sane. How about printf "%d.%d.%d" ${AX_MAJOR_VERSION}...?
• help message is a bit confusing? tags come from the local repository so why are we suggesting a push? Perhaps just give people enough to google their way out.

Playing around I came up with

AC_MSG_CHECKING([whether version number is sane])
AS_IF([printf "%d.%d.%d" ${AX_MAJOR_VERSION} ${AX_MINOR_VERSION} ${AX_POINT_VERSION} >/dev/null 2>&1], [
   AC_MSG_RESULT([yes])
],[
   AC_MSG_RESULT([no])
   AC_MSG_ERROR([Ensure your local git repo contains tags or set FLUX_VERSION.])
])

[wihobbs]

Hey, those macros are neat! Thanks for the suggestions, especially wrt making the check more concise.

I favor a longer message to help dig people out of a hole. The target audience who would be seeing this is new contributors. Not even necessarily new contributors to Flux, we get people (me in 2022, the summer interns, etc.) who are new contributors to any open-source project.

The motivator for adding this is recently it took me almost two hours to debug a version-related failure in sched. An intern was trying to run the testsuite in a CI environment off of his fork before opening a PR. None of the tests would run, and sched wouldn't even build, because CMake kept rejecting the version as invalid. It took me almost two hours to figure out what was going on -- his fork of flux-sched didn't have tags in it because he didn't check that box when he forked. If you run git tag locally, you get the upstream tags, which made this really confusing to debug. We fixed this in his remote CI environment by pushing tags from the upstream flux-sched into his fork, which is why I put that as suggestion no. 3.

Perhaps the message really should read:

      3. If you are running CI in a fork of the main repository, try
         \`git push --tags\` to make sure tags are synchronized in
         your fork.

Does that clarify @garlick? Sorry for the book.

It'd also be nice to keep the messages consistent between flux-core and flux-sched.

[garlick]

Thanks for doing this @wihobbs ! I have some nit picky suggestions about the message but otherwise looks good. Take whatever suggestions you think are good and go ahead and set MWP when ready.

[garlick]

Setting FLUX_VERSION probably ought to be the last option listed since getting it wrong could cause confusion down the line.

[garlick]

I think this applies to any environment, and it would not apply to the native project CI based on github workflows. Drop the "if" qualifier?

[wihobbs]

Oh, also it should be git fetch --tags, whoops!

[garlick]

If "CI" is referencing flux-test-collective's gitlab environment maybe reference that specifically?

[grondo]

I think this is for people using github CI testing on their own fork of a repository, where they may have neglected to push recent tags to their own fork (though I would think when you choose to fork via github you get tags by default, so I'm surprised if a user would hit this one)

[garlick]

Ah I hadn't thought of that one.

[garlick]

Changes LGTM!

[wihobbs]

Thanks @garlick! I meant to say (but forgot to push the "comment" button 🤦) that I wanted to clarify git push --tags to the remote repo and specifically the user's fork. You're right, that was a bit confusing. Setting MWP.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.35%. Comparing base (3405555) to head (8a5f3b4).
Report is 509 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6276      +/-   ##
==========================================
+ Coverage   83.33%   83.35%   +0.02%     
==========================================
  Files         521      521              
  Lines       85793    85793              
==========================================
+ Hits        71495    71516      +21     
+ Misses      14298    14277      -21     

see 10 files with indirect coverage changes