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

build: add descriptive warning about version #1294

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

wihobbs
Copy link
Member

@wihobbs wihobbs commented Sep 6, 2024

Problem: flux-sched's version comes from git describe, which is passed to CMake. CMake validates versions using a regex in CMake/Source/cmProjectCommand.cxx. When this is an invalid version, flux-sched can't build. This often happens in CI, forks, or stripped-down user environments, and often happens to new contributors.

Solution: Validate the version before passing to CMake. If the version is invalid, throw a descriptive warning with some suggestions.

Fixes #1291

This might be a little over-engineered, and suggestions about how to format the message are welcome.

@wihobbs wihobbs requested a review from trws September 6, 2024 17:53
@trws
Copy link
Member

trws commented Sep 6, 2024

It works for me, there are two other options to set it:

  1. -DFLUX_SCHED_VER=<ver> as a cmake argument
  2. FLUX_SCHED_VERSION=<ver> as an env var

in case you'd like to include those.

@wihobbs
Copy link
Member Author

wihobbs commented Sep 6, 2024

Put it at the top, since IMO that's the easiest way to fix it. New message:

(s=125,d=0)  corona212 ~/flux-sched (cmake-ver-err)$ FLUX_SCHED_VERSION=junk cmake -B build
-- VER junk
CMake Warning at CMakeLists.txt:21 (message):
  CMake may generate an error that says "VERSION "junk" format invalid."

  If this happens, try the following:

      1. Set the variable manually, with `cmake -DFLUX_SCHED_VER=<version> ...`
         or FLUX_SCHED_VERSION=<version> in your environment.
      2. If you are running in a CI environment, run `git fetch tags`
         before building. Versions in flux-sched are derived from
         `git describe` which uses the most recent tag.
      3. If you are running in a fork of the main repository, try
         `git push --tags` to make sure tags are synchronized in
         your fork.
      4. If you are running outside of a repo (such as in a buildfarm),
         add a file to the source directory called flux-sched.ver and
         place a valid version string in that file.


CMake Error at CMakeLists.txt:37 (project):
  VERSION "junk" format invalid.


-- Configuring incomplete, errors occurred!
See also "/g/g0/hobbs17/flux-sched/build/CMakeFiles/CMakeOutput.log".

@wihobbs
Copy link
Member Author

wihobbs commented Sep 6, 2024

Oh, grr, git fetch tags needs to be git fetch --tags

@trws
Copy link
Member

trws commented Sep 6, 2024

Looks good to me! Are you doing a similar one on core?

Problem: flux-sched's version comes from `git describe`, which is
passed to CMake. CMake validates versions using a regex in
`CMake/Source/cmProjectCommand.cxx`. When this is an invalid
version, flux-sched can't build. This often happens in CI, forks,
or stripped-down user environments, and often happens to new
contributors.

Solution: Validate the version before passing to CMake. If the
version is invalid, throw a descriptive warning with some
suggestions.

Fixes flux-framework#1291
@wihobbs wihobbs added the merge-when-passing mergify.io - merge PR automatically once CI passes label Sep 9, 2024
@wihobbs
Copy link
Member Author

wihobbs commented Sep 9, 2024

@trws Sure am, made it easier once I realized the configure.ac is really just a shell script with a different suffix.

@mergify mergify bot merged commit 8d5166b into flux-framework:master Sep 9, 2024
21 checks passed
Copy link

codecov bot commented Dec 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.3%. Comparing base (8d8941d) to head (1bcb014).
Report is 53 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1294   +/-   ##
======================================
  Coverage    75.3%   75.3%           
======================================
  Files         110     110           
  Lines       15236   15236           
======================================
  Hits        11486   11486           
  Misses       3750    3750           

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-passing mergify.io - merge PR automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Descriptive error for missing git tags
2 participants