Skip to content

Fix: Clean up command line directory inconsistencies #1079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pniedzielski wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix: Clean up command line directory inconsistencies #1079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4709e51
Fix(bmqbrkr): Make `--config` a required command line argument
pniedzielski Jan 14, 2026
8046e63
Fix(bmqbrkr): Add `--prefixDir` argument to override `BMQ_PREFIX`
pniedzielski Jan 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions src/applications/bmqbrkr/bmqbrkr.m.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ struct TaskEnvironment {
// otherwise)

bsl::string d_bmqPrefix;
// BMQ_PREFIX directory path
// Directory path to store PID, HIST, and CTL files under.

bsl::string d_configJson;
// JSON content ouput of the generated
Expand Down Expand Up @@ -414,7 +414,7 @@ static int initializeTask(bsl::ostream& errorDescription,
bdlf::PlaceHolders::_1, // prefix
bdlf::PlaceHolders::_2)); // istream

// Save the PID of the process in the '${BMQ_PREFIX}/bmqbrkr.pid' file
// Save the PID of the process in the '{prefix}/bmqbrkr.pid' file
const bsl::string pidFile = taskEnv->d_bmqPrefix + "/bmqbrkr.pid";
bsl::ofstream pidFd(pidFile.c_str());
if (!pidFd) {
Expand Down Expand Up @@ -499,9 +499,9 @@ static void shutdownApplication(TaskEnvironment* taskEnv)
app.mqba::Application::~Application();
}

/// Update the `bmqbrkr.hist` file (in the BMQ_PREFIX directory) using the
/// Update the `bmqbrkr.hist` file (in the prefix directory) using the
/// specified `taskEnv`. This file contains information about the last `n`
/// successfull start of the broker, in reverse time order.
/// successful starts of the broker, in reverse time order.
/// Each line entry has the following format:
/// <currentTime_UTC>|<brokerVersion>|<configVersion>|<brokerId>
///
Expand Down Expand Up @@ -598,6 +598,7 @@ int main(int argc, const char* argv[])
{
// Parse command line parameters
bsl::string configDir;
bsl::string prefixDir;
bsl::string instanceId = "default";
bsl::string hostName;
bsl::string hostTags;
Expand All @@ -610,6 +611,11 @@ int main(int argc, const char* argv[])
"config",
"Path to the configuration directory",
balcl::TypeInfo(&configDir),
balcl::OccurrenceInfo::e_REQUIRED},
{"",
"prefixDir",
"Path to the prefix directory (where PID, HIST, and CTL files live)",
balcl::TypeInfo(&prefixDir),
balcl::OccurrenceInfo::e_OPTIONAL},
{"i|instanceId",
"instanceId",
Expand Down Expand Up @@ -658,12 +664,6 @@ int main(int argc, const char* argv[])
return 0;
}

if (configDir.empty()) {
bsl::cerr << "Error: No value supplied for the non-option argument "
"\"config\".\n";
return mqbu::ExitCode::e_COMMAND_LINE; // RETURN
}

printStartStopTrace("STARTING");

ignoreSigpipe();
Expand Down Expand Up @@ -693,8 +693,11 @@ int main(int argc, const char* argv[])
TaskEnvironment taskEnv;
s_taskEnv_p = &taskEnv;

const char* prefixEnvVar = bsl::getenv("BMQ_PREFIX");
taskEnv.d_bmqPrefix = (prefixEnvVar != 0 ? prefixEnvVar : "./");
// Default prefix directory to `BMQ_PREFIX` or ./
if (prefixDir.empty()) {
prefixDir = bsl::getenv("BMQ_PREFIX");
}
Comment on lines +697 to +699

@chrisbeard chrisbeard Jan 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the only real question here is should the env var win even if --prefixDir is provided on the cli? Unsure.

@hallfox hallfox Jan 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually the order of precedence I expect is config file, CLI arg, environment variable in order of lowest to highest precedence (i.e. BMQ_PREFIX should win)

@pniedzielski pniedzielski Jan 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinions either way.

With bmqtool, it looks like the CLI param --broker overrides BMQ_PORT:

blazingmq/src/applications/bmqtool/bmqtool.m.cpp

Line 173 in 112f750

char* bmqPort = bsl::getenv("BMQ_PORT");

But BMQ_BROKER_URI overrides the user's session options:

blazingmq/src/groups/bmq/bmqa/bmqa_session.cpp

Line 298 in 112f750

// Override session options from the environment.

So it's a mess even with the only two examples we have.

@pniedzielski pniedzielski Feb 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a little research, it seems that most other CLI tools have CLI arg as the highest precedence.

  • Docker: https://docs.docker.com/reference/cli/docker/#configuration-files "Command line options override environment variables and environment variables override properties you specify in a config.json file."
  • Git: Does not document this very clearly from what I can tell (https://git-scm.com/docs/git#_environment_variables). But, running the following test confirms that the CLI argument takes higher precedence: 
    cd ~
GIT_DIR=~/blazingmq/.git git --git-dir=~/blazingmq-sdk-java/.git fetch
# remote: Enumerating objects: 23, done.
# remote: Counting objects: 100% (23/23), done.
# remote: Compressing objects: 100% (9/9), done.
# remote: Total 23 (delta 8), reused 13 (delta 8), pack-reused 0 (from 0)
# Unpacking objects: 100% (23/23), 5.93 KiB | 276.00 KiB/s, done.
# From https://github.com/bloomberg/blazingmq-sdk-java
#    9203baf..a342729  main       -> upstream/main
  • HashiCorp Vault: I think this is a bit more like BlazingMQ as a network server. https://developer.hashicorp.com/vault/docs/commands#configure-environment-variables "You can use environment variables to configure the CLI globally. Some configuration settings have a corresponding CLI flag to configure a specific command. For example, export VAULT_ADDR='http://localhost:8200' sets the address of your Vault server globally, while -address='http://someotherhost:8200' overrides the value for a specific command."

I have not been able to find anything that follows the precedence config file < CLI arg < environment variable.

Second thing I've found is it's hard to find this documented explicitly. So I think we should document explicitly what we choose.

@hallfox hallfox Mar 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to this after two months but I have no idea what I was thinking when I wrote this originally. Most tools I use do the config file < environment var < CLI arg order. Maybe I was thinking about how one might control broker configurations from a docker image?

Either way I do think the most important thing is to document the precedence regardless of what takes precedent. I just don't agree with what I originally wrote anymore.

taskEnv.d_bmqPrefix = (!prefixDir.empty() ? prefixDir : "./");
taskEnv.d_instanceId = instanceId;

bmqu::MemOutStream errorDescription;
Expand Down
2 changes: 1 addition & 1 deletion src/applications/bmqbrkr/m_bmqbrkr_task.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ class Task {
// True is this object has been initialized.

bsl::string d_bmqPrefix;
// BMQ_PREFIX directory
// Directory path to store PID, HIST, and CTL files under.

bdlmt::EventScheduler d_scheduler;
// EventScheduler
Expand Down
Loading