1902: upgrade `CLI11` to `v2.2.0` #1904

cz4rs · 2022-08-09T11:24:48Z

Upgrade CLI11 to the latest release.
Note: this keeps the added namespace vt in the header.

fixes #1902

src/vt/configs/arguments/args.cc

github-actions · 2022-08-09T11:28:18Z

Pipelines results

PR tests (gcc-5, ubuntu, mpich)

Build for b7af7e5

Compilation - successful

Testing - passed

Build log

PR tests (clang-3.9, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (gcc-6, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (clang-5.0, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (gcc-9, ubuntu, mpich, zoltan)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (gcc-8, ubuntu, mpich, address sanitizer)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (gcc-10, ubuntu, openmpi, no LB)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (gcc-7, ubuntu, mpich, trace runtime, LB)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 10.1, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (clang-9, ubuntu, mpich)

Build for b7af7e5

Compilation - successful

Testing - passed

Build log

PR tests (clang-13, alpine, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (clang-11, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (intel icpx, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (clang-12, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 11.0, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (clang-14, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (gcc-11, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (clang-13, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (intel icpc, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (gcc-12, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

PR tests (clang-10, ubuntu, mpich)

Build for f233f14

Compilation - successful

Testing - passed

Build log

codecov · 2022-08-09T11:32:08Z

Codecov Report

Merging #1904 (f233f14) into develop (0633bc9) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1904      +/-   ##
===========================================
+ Coverage    84.35%   84.44%   +0.08%     
===========================================
  Files          760      760              
  Lines        26834    26833       -1     
===========================================
+ Hits         22636    22658      +22     
+ Misses        4198     4175      -23

Impacted Files	Coverage Δ
src/vt/configs/arguments/args.cc	`94.57% <100.00%> (-0.02%)`	⬇️
src/vt/vrt/collection/manager.impl.h	`96.48% <0.00%> (+0.98%)`	⬆️
src/vt/topos/location/location.impl.h	`94.09% <0.00%> (+5.90%)`	⬆️

PhilMiller

This all looks very nice.

Could you confirm that all of the defaults actually matched?

nmm0

Looks good to me, also fixes C++17 compile issue

PhilMiller

Please work around the Intel CI failure before merging, so that we don't just see that failure on every PR going forward

PhilMiller · 2022-08-09T17:18:13Z

FAILED: src/CMakeFiles/vt.dir/Unity/unity_0_cxx.cxx.o 
/usr/bin/ccache /usr/local/bin/mpicxx -DJSON_USE_IMPLICIT_CONVERSIONS=1 -DVT_NO_COLOR_ENABLED -I/vt/lib/CLI -I/build/vt/release -I/vt/src -I/vt/lib/json/include -I/vt/lib/brotli/c/include -I/vt/lib/libfort/lib -isystem /vt/lib/fmt/include -isystem /vt/lib/EngFormat-Cpp/include -isystem /build/checkpoint/install/include -isystem /build/detector/install/include -O3 -DNDEBUG -Wall -pedantic -Wshadow -Wno-unknown-pragmas -Wsign-compare -Werror -fPIC -std=c++14 -MD -MT src/CMakeFiles/vt.dir/Unity/unity_0_cxx.cxx.o -MF src/CMakeFiles/vt.dir/Unity/unity_0_cxx.cxx.o.d -o src/CMakeFiles/vt.dir/Unity/unity_0_cxx.cxx.o -c /build/vt/src/CMakeFiles/vt.dir/Unity/unity_0_cxx.cxx
In file included from /vt/src/vt/configs/arguments/args.cc(54),
                 from /build/vt/src/CMakeFiles/vt.dir/Unity/unity_0_cxx.cxx(9):
/vt/lib/CLI/CLI/CLI11.hpp(8143): error #3280: declaration hides variable "optarg" (declared at line 36 of "/usr/include/x86_64-linux-gnu/bits/getopt_core.h")
                      std::string optarg = args.back();
                                  ^

cz4rs · 2022-08-10T17:37:56Z

lib/CLI/CLI/CLI11.hpp

-                    std::string optarg = args.back();
-                    optarg = op->_validate(optarg, 0);
-                    if(!optarg.empty()) {
+                    std::string cli_optarg = args.back();


I think that's better than adding Wno-shadow for icpc.
Btw this is already fixed at main branch of CLI11.

src/vt/configs/arguments/args.cc

cz4rs · 2022-08-10T17:42:20Z

src/vt/configs/arguments/args.cc

-  auto m = app.add_option("--vt_stack_mod",     appConfig.vt_stack_mod,       mod,  1);
+  auto k = app.add_option("--vt_stack_file",    appConfig.vt_stack_file,      file)->capture_default_str();
+  auto l = app.add_option("--vt_stack_dir",     appConfig.vt_stack_dir,       dir)->capture_default_str();
+  auto m = app.add_option("--vt_stack_mod",     appConfig.vt_stack_mod,       mod)->capture_default_str();


Difference:

vt/src/vt/configs/arguments/app_config.h

Line 117 in e8ca3b5

int32_t vt_stack_mod = 0;

cz4rs · 2022-08-10T17:43:04Z

src/vt/configs/arguments/args.cc

-  auto qf = app.add_option("--vt_trace_flush_size",      appConfig.vt_trace_flush_size,        tflushmod, 0);
+  auto o  = app.add_option("--vt_trace_file",            appConfig.vt_trace_file,              tfile)->capture_default_str();
+  auto p  = app.add_option("--vt_trace_dir",             appConfig.vt_trace_dir,               tdir)->capture_default_str();
+  auto q  = app.add_option("--vt_trace_mod",             appConfig.vt_trace_mod,               tmod)->capture_default_str();


Difference:

vt/src/vt/configs/arguments/app_config.h

Line 129 in e8ca3b5

int32_t vt_trace_mod = 0;

cz4rs · 2022-08-10T17:45:11Z

src/vt/configs/arguments/args.cc

-  auto xy = app.add_option("--vt_lb_data_file_in", appConfig.vt_lb_data_file_in, lb_data_file_in, lbs);
+  auto wx = app.add_option("--vt_lb_data_dir", appConfig.vt_lb_data_dir, lb_data_dir)->capture_default_str();
+  auto wy = app.add_option("--vt_lb_data_file", appConfig.vt_lb_data_file, lb_data_file)->capture_default_str();
+  auto xx = app.add_option("--vt_lb_data_dir_in", appConfig.vt_lb_data_dir_in, lb_data_dir_in)->capture_default_str();


vt/src/vt/configs/arguments/app_config.h

Line 149 in e8ca3b5

std::string vt_lb_data_file = "data.%p.json";

vs

auto lbd = "vt_lb_data";

cz4rs · 2022-08-10T17:46:02Z

src/vt/configs/arguments/args.cc

+  auto wx = app.add_option("--vt_lb_data_dir", appConfig.vt_lb_data_dir, lb_data_dir)->capture_default_str();
+  auto wy = app.add_option("--vt_lb_data_file", appConfig.vt_lb_data_file, lb_data_file)->capture_default_str();
+  auto xx = app.add_option("--vt_lb_data_dir_in", appConfig.vt_lb_data_dir_in, lb_data_dir_in)->capture_default_str();
+  auto xy = app.add_option("--vt_lb_data_file_in", appConfig.vt_lb_data_file_in, lb_data_file_in)->capture_default_str();


vt/src/vt/configs/arguments/app_config.h

Line 151 in e8ca3b5

std::string vt_lb_data_file_in = "data.%p.json";

vs

auto lbs = "data";

cz4rs · 2022-08-10T17:49:34Z

This all looks very nice.

Could you confirm that all of the defaults actually matched?

I have double-checked and noted the differences in the comments.

icpc 2021.6.0 shows following warning: `#3280: declaration hides variable "optarg" (declared at line 36 of "/usr/include/x86_64-linux-gnu/bits/getopt_core.h")`

cz4rs · 2022-08-11T15:18:25Z

Rebased on top of develop.
All the differences regarding default values should only make the --vt_help output more accurate (the ones in args.cc were out of sync).

cz4rs commented Aug 9, 2022

View reviewed changes

src/vt/configs/arguments/args.cc Show resolved Hide resolved

PhilMiller reviewed Aug 9, 2022

View reviewed changes

nmm0 approved these changes Aug 9, 2022

View reviewed changes

PhilMiller requested changes Aug 9, 2022

View reviewed changes

nmm0 mentioned this pull request Aug 9, 2022

Meeting Agenda [do not close] #925

Open

cz4rs requested review from jstrzebonski, nlslatt, stmcgovern, JacobDomagala, lifflander, nmm0, PhilMiller and thearusable August 10, 2022 17:36

cz4rs commented Aug 10, 2022

View reviewed changes

src/vt/configs/arguments/args.cc Show resolved Hide resolved

cz4rs commented Aug 10, 2022

View reviewed changes

cz4rs added 2 commits August 11, 2022 15:50

#1902: upgrade CLI11 to v2.2

6396a30

#1902: patch CLI11 to avoid icpc shadow warning

f233f14

icpc 2021.6.0 shows following warning: `#3280: declaration hides variable "optarg" (declared at line 36 of "/usr/include/x86_64-linux-gnu/bits/getopt_core.h")`

cz4rs force-pushed the 1902-upgrade-cli11-lib branch from b7af7e5 to f233f14 Compare August 11, 2022 13:50

PhilMiller approved these changes Aug 11, 2022

View reviewed changes

PhilMiller merged commit e89215e into develop Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1902: upgrade `CLI11` to `v2.2.0` #1904

1902: upgrade `CLI11` to `v2.2.0` #1904

cz4rs commented Aug 9, 2022

github-actions bot commented Aug 9, 2022 •

edited

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading

PhilMiller left a comment

nmm0 left a comment

PhilMiller left a comment

PhilMiller commented Aug 9, 2022

cz4rs Aug 10, 2022

cz4rs Aug 10, 2022

cz4rs Aug 10, 2022

cz4rs Aug 10, 2022 •

edited

Loading

cz4rs Aug 10, 2022 •

edited

Loading

cz4rs commented Aug 10, 2022

cz4rs commented Aug 11, 2022

1902: upgrade CLI11 to v2.2.0 #1904

1902: upgrade CLI11 to v2.2.0 #1904

Conversation

cz4rs commented Aug 9, 2022

github-actions bot commented Aug 9, 2022 • edited Loading

Pipelines results

codecov bot commented Aug 9, 2022 • edited Loading

Codecov Report

PhilMiller left a comment

Choose a reason for hiding this comment

nmm0 left a comment

Choose a reason for hiding this comment

PhilMiller left a comment

Choose a reason for hiding this comment

PhilMiller commented Aug 9, 2022

cz4rs Aug 10, 2022

Choose a reason for hiding this comment

cz4rs Aug 10, 2022

Choose a reason for hiding this comment

cz4rs Aug 10, 2022

Choose a reason for hiding this comment

cz4rs Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

cz4rs Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

cz4rs commented Aug 10, 2022

cz4rs commented Aug 11, 2022

1902: upgrade `CLI11` to `v2.2.0` #1904

1902: upgrade `CLI11` to `v2.2.0` #1904

github-actions bot commented Aug 9, 2022 •

edited

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading

cz4rs Aug 10, 2022 •

edited

Loading

cz4rs Aug 10, 2022 •

edited

Loading