Skip to content

update RunTool for new run_info database schema#1861

Open
rlcee wants to merge 5 commits into
Mu2e:mainfrom
rlcee:ri_260613
Open

update RunTool for new run_info database schema#1861
rlcee wants to merge 5 commits into
Mu2e:mainfrom
rlcee:ri_260613

Conversation

@rlcee

@rlcee rlcee commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

The run_info database schema ahs been developed over the last few months. It is at a stable point and so in this PR we adapt the RunTool to the new schema. The content is basically the same, but the tables have be reorganized. The old schema is not supported. Several classes are added to represent the new organization. The command line tool has the same interface, but the output is updated. RunTool can be include in c++ code and used anywhere. The python interface generated by pywrap has been tested. This update is necessary to move ahead pass1 automation. There should be no effect on validation or operations, so low risk merge.

@FNALbuild

Copy link
Copy Markdown
Collaborator

Hi @rlcee,
You have proposed changes to files in these packages:

  • DbService
  • GeneralUtilities

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for 7e3c704: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild

Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 7e3c704.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 7e3c704 at 2bf2d3b
build (prof) Log file. Build time: 04 min 24 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
trigger Log file.
check_cmake Log file.
FIXME, TODO TODO (0) FIXME (0) in 11 files
clang-tidy ➡️ 0 errors 65 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 7e3c704 after being merged into the base branch at 2bf2d3b.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@oksuzian

Copy link
Copy Markdown
Collaborator

1. 🔴 setenv("TZ", …) is never restored — global side effect (RunTool.cc, printRun)

      // Set timezone to Central US (America/Chicago)
      setenv("TZ", "America/Chicago", 1);
      tzset();
      std::strftime(start_buf, sizeof(start_buf), "%Y-%m-%d %H:%M:%S",
                    std::localtime(&start_t));

RunTool is advertised as includable "anywhere" in C++. Permanently mutating the process TZ env var (and not restoring it) will silently corrupt any later time handling in the host program. It's also set inside the per‑subrun loop, so it runs on every iteration. Fix: save/restore the prior TZ, or use a thread‑safe localtime_r/gmtime_r without touching the environment, and do it once outside the loop.

2. 🟠 nEvents()/minEwt() etc. return long but setters parse std::stol, while time fields are truncated to int

  int startTimeUnix() const { return _start_time_unix; }
  int stopTimeUnix() const { return _stop_time_unix; }

_start_time_unix / _stop_time_unix are stored as int and parsed with std::stoi. These are Unix timestamps — already in the 1.7‑billion range, well within int32 for now, but this is exactly the pattern that bites on 32‑bit time_t/2038. Since printRun assigns them to std::time_t, prefer storing as long/std::time_t and parsing with std::stol for consistency with the other count fields.

3. 🟠 version is read but never populated for configs

RunConfig has version() and printRun prints config.version(), but in listRuns the config parser only sets run_number, subsystem, and settings — setVersion() / setCreateTime() are never called. The custom CSV splitter stops after extracting settings and discards the trailing create_time,version fields, so config version will always print as 0.

            RunConfig config;
            if (!run_num_str.empty())
              config.setRunNumber(std::stoi(run_num_str));
            config.setSubsystem(subsystem);
            config.setSettings(settings);
            runInfo.addConfig(config);

4. 🟡 Subrun field ordering looks inconsistent / fragile

In the subrun parser, nNull is read from index 11 while eventModeCounts (index 9) and createdAt (index 10) are read positionally without the "None" guard used by every numeric field. If the DB column order isn't exactly what's assumed, values land in the wrong fields silently (rows are accepted with only size() >= 2). Worth a comment documenting the exact expected column order, mirroring the one added for the run table.

5. 🟡 Hand-rolled CSV/JSON parsing is duplicated and brittle

The config block manually scans quotes/escapes and runTool_main.cc re-implements JSON pretty-printing by hand. This is a lot of bespoke string logic that splitString was presumably meant to handle (the comment says JSONB escapes force the manual path). Not a blocker, but a likely future maintenance/bug source — a single CSV-field helper would reduce risk.

@rlcee

rlcee commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator Author

AI comments addressed

  1. reasonable comment, fixed
  2. this reflects the schema, will work to fix, OK for now
  3. lazy AI (and, I guess, operator) fixed
  4. hardened all select statements
  5. much better now, hacks removed

@rlcee

rlcee commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator Author

@FNALbuild run build test

@FNALbuild

Copy link
Copy Markdown
Collaborator

⌛ The following tests have been triggered for 26028b6: build (Build queue - API unavailable)

@FNALbuild

Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 26028b6.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 26028b6 at 27e994d
build (prof) Log file. Build time: 08 min 36 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
trigger Log file.
check_cmake Log file.
FIXME, TODO TODO (0) FIXME (0) in 13 files
clang-tidy ➡️ 0 errors 83 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 26028b6 after being merged into the base branch at 27e994d.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants