[Breaking] Prefix `amrex_` to each plotfile Tool #3600

ax3l · 2023-10-18T17:49:17Z

Summary

Packaging AMReX for Conda, we received feedback that our plotfile tool names are too generic and can collide with other packages.

Thus, we propose to rename them with a common prefix.

decide prefix or suffix (suffix: <TAB> completion will work; prefix: easier for new users)
decide name: plt/amrex/amr/...
- Weiqun and Axel prefer amrex_ so far
also change for GNUmake
- update test harness
update user-facing documentation for tools
~~update tutorials?~~

Additional background

conda-forge/staged-recipes#24294

Checklist

The proposed changes:

fix a bug or incorrect behavior in AMReX
add new capabilities to AMReX
changes answers in the test suite to more than roundoff level
are likely to significantly affect the results of downstream AMReX users
include documentation in the code and/or rst files, if appropriate

Tools/Plotfile/CMakeLists.txt

A later, breaking change will introduce tools: AMReX-Codes/amrex#3600 Co-authored-by: Isuru Fernando <[email protected]>

isuruf · 2023-10-19T02:32:03Z

Tools/Plotfile/CMakeLists.txt

+    # Add suffix to each tool's name to make them unique when installed.
+    # This avoids potential collisions of names on user systems, e.g., in
+    # software packages (Spack/Conda/Debian/...).
+    set_target_properties(${_exe} PROPERTIES OUTPUT_NAME "${_exe}_amrex")


Wouldn't this change the executable name from foo.exe to foo_amrex? i.e. drop the .exe suffix?

The old name does not have .exe either.

I think using prefix here is better because all the amrex files will then be grouped together by default (by ls for example).

The output name is before the executable suffix in cmake. It will keep .exe on Windows and none on Unix.

Refs.:

https://cmake.org/cmake/help/v3.28/prop_tgt/OUTPUT_NAME.html#output-name

https://cmake.org/cmake/help/v3.28/prop_tgt/SUFFIX.html#suffix

Packaging AMReX for Conda, we received feedback that our plotfile tool names are too generic and can collide with other packages. Thus, we propose to rename them with a common prefix.

ax3l · 2023-10-19T18:41:46Z

This change might break workflows, in particular:

tests
tutorials
Python test harness by @zingale

and coordinated updates are welcome after/with merge :)

WeiqunZhang · 2023-10-19T19:40:34Z

We do not need to change the GNU Make build. Currently the names are like fcompare.gnu.ex for GNU Make. So they have never consistent.

As for documentation, we only used gnumake as an example. https://amrex-codes.github.io/amrex/docs_html/Post_Processing.html#fextract Maybe we can add a paragraph about building these tools with cmake. We have already mentioned the build option here https://amrex-codes.github.io/amrex/docs_html/BuildingAMReX.html#id2. Maybe we can say something about they will be installed at <amrex_installation_directdory>/bin/ and what the names are.

WeiqunZhang · 2023-10-19T19:44:19Z

I did a case insensitive grep of plotfile_tools, tools/plotfile and fcompare in amrex-tutorials. Nothing showed up.

WeiqunZhang · 2023-10-19T19:53:13Z

Re: regression_testing, even when cmake is used to build the application code and amrex, we still use gnumake built plotfile tools. See https://ccse.lbl.gov/pub/RegressionTesting1/MFIX-Exa/2023-10-19/BENCH01-Size0001.html.

Compilation command:
cmake_3.20 --build /scratch/AMReX_RegTesting/mfix/builddir -j 8 -- mfix

...

/scratch/AMReX_RegTesting/amrex/Tools/Plotfile//fcompare.gnu.ex ...

WeiqunZhang · 2023-10-19T20:05:20Z

@jrood-nrel amr-wind and pele need to be updated after this is merged.

WeiqunZhang · 2023-10-19T20:07:04Z

@ax3l if we do not change gnu make, this PR should be ready for merge.

maxpkatz · 2023-10-19T20:08:03Z

I support leaving GNU make alone.

ax3l · 2023-10-20T05:39:44Z

I think producing different artifacts for the different build systems - and documenting the tools with inconsistent names - will cause a lot of confusion. Especially since we build with CMake for all user-facing package managers (e.g., Spack, Conda).

#3600 (comment)
That sounds good. The tools will actually be in PATH after install and generally available to users.
If we change the docs to (prominently/primarily? because users) refer to the CMake names that people will encounter, and leave GNUmake for hard-core devs with legacy names, I would feel better as well. But my primary concern is users getting confused.

In the end, we can avoid this though and future confusion by synchronizing the tool names...

WeiqunZhang · 2023-10-20T16:08:33Z

I don't think the potential confusion is an issue if we clearly document it, especially since they end up in very different directories. The names have always been inconsistent. I think should just leave GNU make alone.

In any case, we need to update the documentation. Do you want to do it in this PR or a separate PR?

ax3l · 2023-11-01T22:10:00Z

Currently on an airplane. Let's merge this and have a follow-up PR on docs? I can do that end of the week.

## Summary Packaging AMReX for Conda, we received feedback that our plotfile tool names are too generic and can collide with other packages. Thus, we propose to rename them with a common prefix. - [x] decide prefix or suffix (suffix: `<TAB>` completion will work; prefix: easier for new users) - [x] decide name: `plt`/`amrex`/`amr`/... - Weiqun and Axel prefer `amrex_` so far - [ ] also change for GNUmake - [ ] update [test harness](https://github.com/AMReX-Codes/regression_testing) - [ ] update user-facing documentation for tools - [x] ~update [tutorials](https://github.com/AMReX-Codes/amrex-tutorials)?~ ## Additional background conda-forge/staged-recipes#24294 ## Checklist The proposed changes: - [ ] fix a bug or incorrect behavior in AMReX - [ ] add new capabilities to AMReX - [ ] changes answers in the test suite to more than roundoff level - [ ] are likely to significantly affect the results of downstream AMReX users - [ ] include documentation in the code and/or rst files, if appropriate --------- Co-authored-by: Weiqun Zhang <[email protected]>

ax3l requested review from WeiqunZhang and zingale October 18, 2023 17:49

ax3l assigned WeiqunZhang Oct 18, 2023

ax3l commented Oct 18, 2023

View reviewed changes

Tools/Plotfile/CMakeLists.txt Outdated Show resolved Hide resolved

ax3l force-pushed the plotfile-tools-suffix branch from e53009f to 95a86c7 Compare October 18, 2023 18:34

ax3l added a commit to ax3l/staged-recipes that referenced this pull request Oct 18, 2023

Tools: OFF, Win: CPU-Count Variable

f39a06a

A later, breaking change will introduce tools: AMReX-Codes/amrex#3600 Co-authored-by: Isuru Fernando <[email protected]>

isuruf reviewed Oct 19, 2023

View reviewed changes

[Breaking] Prepend amrex_ to each plotfile Tool

54c7cf0

Packaging AMReX for Conda, we received feedback that our plotfile tool names are too generic and can collide with other packages. Thus, we propose to rename them with a common prefix.

ax3l force-pushed the plotfile-tools-suffix branch from 95a86c7 to 54c7cf0 Compare October 19, 2023 17:31

ax3l changed the title ~~[Breaking] Append _amrex to each plotfile Tool~~ [Breaking] Prefix amrex_ to each plotfile Tool Oct 19, 2023

ax3l added the enhancement label Oct 19, 2023

ax3l requested a review from atmyers October 19, 2023 18:41

ax3l mentioned this pull request Oct 19, 2023

Build Plotfile Tools conda-forge/amrex-feedstock#3

Open

Update CIs

aec4689

WeiqunZhang approved these changes Oct 19, 2023

View reviewed changes

WeiqunZhang merged commit 606a94c into AMReX-Codes:development Nov 1, 2023
69 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Breaking] Prefix `amrex_` to each plotfile Tool #3600

[Breaking] Prefix `amrex_` to each plotfile Tool #3600

ax3l commented Oct 18, 2023 •

edited

Loading

isuruf Oct 19, 2023

WeiqunZhang Oct 19, 2023

WeiqunZhang Oct 19, 2023

ax3l Oct 19, 2023 •

edited

Loading

ax3l commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

maxpkatz commented Oct 19, 2023

ax3l commented Oct 20, 2023 •

edited

Loading

WeiqunZhang commented Oct 20, 2023

ax3l commented Nov 1, 2023 •

edited

Loading

[Breaking] Prefix amrex_ to each plotfile Tool #3600

[Breaking] Prefix amrex_ to each plotfile Tool #3600

Conversation

ax3l commented Oct 18, 2023 • edited Loading

Summary

Additional background

Checklist

isuruf Oct 19, 2023

Choose a reason for hiding this comment

WeiqunZhang Oct 19, 2023

Choose a reason for hiding this comment

WeiqunZhang Oct 19, 2023

Choose a reason for hiding this comment

ax3l Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

ax3l commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

WeiqunZhang commented Oct 19, 2023

maxpkatz commented Oct 19, 2023

ax3l commented Oct 20, 2023 • edited Loading

WeiqunZhang commented Oct 20, 2023

ax3l commented Nov 1, 2023 • edited Loading

[Breaking] Prefix `amrex_` to each plotfile Tool #3600

[Breaking] Prefix `amrex_` to each plotfile Tool #3600

ax3l commented Oct 18, 2023 •

edited

Loading

ax3l Oct 19, 2023 •

edited

Loading

ax3l commented Oct 20, 2023 •

edited

Loading

ax3l commented Nov 1, 2023 •

edited

Loading