visit-dav / visit

VisIt - Visualization and Data Analysis for Mesh-based Scientific Data
https://visit.llnl.gov
BSD 3-Clause "New" or "Revised" License
444 stars 117 forks source link

bv_mylib script variable convention #5617

Open ARSanderson opened 3 years ago

ARSanderson commented 3 years ago

In the past for thirdparty libraries source has been downloaded and binaries built along side the source. More recently out of source builds are done or thirdparty libraries may be prebuilt. As such, using MYLIB_BUILD_DIR is not always accurate.

For consistency we should use the following: MYLIB_SRC_DIR - source code from a tarball MYLIB_BUILD_DIR - could be the the source code dir or an out of source dir MYLIB_BINARY_DIR - binary code from a tarball MYLIB_INSTALL_DIR - the thirdparty install dir for this lib - third_party/mylib/version/os/

Currently, when unpacking a tarball, we assume an in source build and MYLIB_BUILD_DIR initially which lead to naming helper functions: prepare_build_dir. Neither of which is accurate as that function just unpacks the tar ball.

As such, I would like to propose the following:

  1. Rename prepare_build_dir to uncompress_file or unpack_file
  2. If the library contains binary files to be copied over MYLIB_BINARY_DIR is used (SRC and BUILD are not used).
  3. If building within source only MYLIB_SRC_DIR is used throughout (MYLIB_BUILD_DIR is the current standard)
  4. If building out of source only MYLIB_BUILD_DIR and MYLIB_SRC_DIR is set
  5. Create a template bv_file that is part of the repo. The current bv_files have been copied ad nauseum that there is no standard across all of them.

For 2 I have already cleaned up those that are binary.

For 3 There are over 60 bv_.sh files. Of those there are only five bv.sh files that do out of source builds (see below). I think for all others MYLIB_SRC_DIR should be used because that makes it clear that the build is in the SRC dir.

For 4 both MYLIB_SRC_DIR and MYLIB_BUILD_DIR really should be set up front. I have cleaned up: PIDX, VTK, conduit, nektar++- and I have already created another with the new standard.

For 5 VTK is probably the most reasonable to become a standard as it has patches, src options, etc.

ARSanderson commented 3 years ago

@biagas, @cyrush and @markcmiller86 As you all have mucked about a bit over the years with the bv* file could I get some input from you all. I did this initial clean up as I was building VTK with OSPRay as new bv* were needed. The input needed most specifically is 3. which changes the current name paradigm. I am not set that it should be and it is easy to accomplish with a script. Thanks

biagas commented 3 years ago

For 3: I am fine with the name change. I think it makes a lot of sense, and have often thought to do the same when I encountered the strange logic used for out-of-source-builds to rename build dir. You've accounted for calls to 'ensure_built_or_ready' ?

For 5: there is a a script to create a basic bv_module: construct_build_visit_module.sh. It may need an update. I suggest just updating that.

ARSanderson commented 3 years ago

@biagas - Ahh yes, ensure_built_or_ready should really be ensure_installed_or_ready and I see some other variable name changes that should be made for clarity.

markcmiller86 commented 3 years ago

@ARSanderson as I read these comments, they appear to be referring to (some) work you've already done that we can't see. I think the GitHub way of engaging a conversation about that is to use a draft pull request. That way, we can all see and have a conversation about it.

If it is at all practical, can you push to origin and create a draft PR here to continue this conversation?

ARSanderson commented 3 years ago

@markcmiller86 correct some work has been done. Actually at this point most has been done and is now in this branch.

task/allen/bv_cleanup

A good example to look at is bv_vtk.sh.

BTW when building vtk, vtkm and vtkh if the source dirs are present they get deleted and then uncompress again. Not sure if anyone ever noticed that. That is ensure_built_or_ready is called twice once in the bv_vtk_ensure and once in the build_vtk

markcmiller86 commented 3 years ago

I think any one of us can certainly do a git checkout task/allen/bv_cleanup and a git pull to go look at things there.

But, again, that doesn't really facilitate a conversation about the proposed changes where comments/questions are intermixed with file diffs.

Is submitting a draft PR to facilitate this conversation possible?

ARSanderson commented 3 years ago

@markcmiller86 - see https://github.com/visit-dav/visit/pull/5653

markcmiller86 commented 3 years ago

Aweseome! @ARSanderson thanks so much! Much easier to talk about the details now.

markcmiller86 commented 3 years ago

Ok, I did two things. I made a lengthy comment in the PR but then also 14 comments pinned to lines changed for concrete conversations. I hope this helps get the dialog going, Allen. Thanks for starting this work.