build_visit improvements for patching

[markcmiller86]

Proposed best practices for handling patching in build_visit

• Only use patch -p0 << \EOF not a bare EOF without the leading backslash.
  • Rationale: \EOF is necessary to patch code involving $ chars (e.g. shell code)
• Maximally restrict patching so that patches are applied under only the conditions we have verified they are necessary. For example, if a package builds fine everywhere except macOS, ensure the patch is restricted to macOS.
  • Rationale: should be self-evident...helps to minimize non-standard builds
• Put unrelated patches in different bash functions
• Name patch functions by package's version number and purpose (e.g. apply_cgns_410_missing_ldflags_patch)
  • Rationale: A given version of a TPL can require multiple patches to fix different issues
• Put logic effecting when a patch is applied in callers of the patch functions and not the patch functions themselves
  • Rationale: makes it a bit clearer to understand when patches get applied
• Use unified diff format for diffs used in patches (e.g. diff -u)
  • Rationale: Unified diffs are quite a bit shorter
• Put messaging related to patching in the patch functions and not the callers of those functions
  • Rationale: encpasulation
• Invoke patches after cd'ing to build dir
  • Rationale: ensures some level of consistency of how patches get defined
• Define patches relative to the build dir of the package
  • What about out of source builds?
• When libs are updated, old patch logic should be removed
  • Rationale: should be self-evident. When we bump the version of a lib, we no longer need to maintain patches for the older version.

The above would help to unify build_visit where patching is concerned.

[brugger1]

Before removing a patch, we should make sure it is no longer needed. We've had several instances of patches being removed that were fixing bugs rather than compile issues that caused non-obvious bugs to reappear.

[markcmiller86]

• Logic that uses failure of one patch to discontinue all patches should be removed
• Some patches are fixing a bug and should be everywhere
• VTK example of ospray patch getting stopped when another patch failed
• When we've untarr'ed a package, we patch it and don't need to patch. We think ospray may be an example of where we don't do this.
• If we move to a newer version of a package in build visit, then we should remove all the logic related to the old version including patching logic.
• patches should be defined relative to root of src/build dir not root of build_visit dir
• native support for out of source builds (BUILD_DIR and SOURCE_DIR vars) which for some packages may be the same.
• indented patches is a bad idea. They should be cut-n-paste verbatim (raw output from diff -u)
• The symbol names XXX_BUILD_DIR is actually representing more related to a source dir
• option to -force rebuild from source
• don't just remove CMake cache files to restart a package build...remove the whole build dir.
• have a look a work from @ARSanderson on build_visit from these commits and in particular improvements in the names of functions.
• we should document all these practices and rationales.

[biagas]

It occurs to me this ticket initially addressed only patching within build_visit modules, and I added the improvements suggested by @ARSanderson. Should that be a separate ticket?

I looked at the closed PR related to this (#5653) and I think the main reason it never got merged is due to the number of library version updates that were added in addition to the build_visit cleanup.

His changes to helper_funcs.sh I think look good, the name changes to functions and variables makes sense.

His changes to construct_build_visit_module.sh are helpful, but the main build_xxx module assumes a cmake build system that not all libraries support. Should this module constructor helper have both a cmake pattern and a configure pattern?

The rest of the changes are just to get the existing modules to conform to the new naming schemes (except of course those libraries whose versions were also updated).

[markcmiller86]

I think all of these comments will result in a set of improvements/fixes for build_visit as new issues whether they are specific to patching practices or not.

[cyrush]

We should do a code sprint. We need build_visit testing to help with.