Skip to content
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.

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

HammerDBcli does not work correctly when added to PATH and called from other directory #597

Open
akantak opened this issue Sep 14, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@akantak
Copy link

akantak commented Sep 14, 2023

Is your feature request related to a problem? Please describe.
When adding HammerDB home to the PATH (on Linux), and calling hammerdbcli from a different directory, it throws an error, as it is using relative paths, like ./bin/tclsh8.6

$ echo $PATH
(...):/home/akantak/HammerDB-4.8
$ pwd
/home/akantak
$ hammerdbcli
/home/akantak/HammerDB-4.8/hammerdbcli: 12: exec: ./bin/tclsh8.6: not found
$

1. Describe the solution you'd like
Add code to change the directory to HammerDB "home", in hammerdb, hammerdbcli and hammerdbws, right at the beginning of the scripts, in line 3. Example code:

## \
cd "$(dirname "$0")"

2. Describe alternatives you've considered
The other approach here can be to define in the same line 3 a variable called HAMMERDB_HOME, and change all relative paths to full paths, like ${HAMMERDB_HOME}/bin/tclsh8.6 instead of ./bin/tclsh8.6, but it would require more changes in the code.
3. Describe alternatives you've considered
A third approach could be to suggest adding to the path both ${HAMMERDB_HOME} and ${HAMMERDB_HOME}/bin and get rid of relative paths, using tclsh8.6 instead of ./bin/tclsh8.6. But it can stop working when somebody has not added Hammerdb to PATH.

Additional context
I can submit a PR on my own, but wanted to discuss a solution first.

@sm-shaw sm-shaw added the enhancement New feature or request label Sep 15, 2023
@sm-shaw
Copy link
Contributor

sm-shaw commented Sep 15, 2023

Yes, HammerDB is intended to be run from the home directory which mostly to now has not caused a major issue and has not been a priority to change.

You are welcome to submit a PR to enable calling by absolute path although some things to note first are that firstly modifications would be need to hammerdb, hammerdbcli, hammerdbws and agent on Linux and the equivalent .bat files on Windows including the additional mpstat.bat file on Windows.

Also note that in the scripts the virtual users are also loading libraries, modules and packages from their relative directories e.g.

#LOAD LIBRARIES AND MODULES
if [catch {package require $library $version} message] { error "Failed to load $library - $message" }
if [catch {::tcl::tm::path add modules} ] { error "Failed to find modules directory" }
if [catch {package require tpcccommon} ] { error "Failed to load tpcc common functions" } else { namespace import tpcccommon::* }

The virtual users run completely in parallel, so maintain their own separate environments.

Therefore, probably the best solution if this is desired is the first suggestion to cd to the HammerDB directory if you are not already there first. Then it is least likely to break existing functionality. This will need to be done and checked for all the files mentioned above on both Linux and Windows.

@akantak
Copy link
Author

akantak commented Sep 21, 2023

I submitted a draft, but I have still one doubt.
in agent/agent line 10, it can be either ${0##*/} so extracting file name only from the path, or hardcoded ./agent like it is in agent/agent.bat. Because when we call, ex. ./agent/agent, but change dir, this path is no longer valid (but used as $0), and should be changed to ./agent.
Also, not sure if anybody would start an agent per se, with an added ${HAMMERDB}/agent to the path too. Is it a valid use case @sm-shaw ?

Until we figure this out, I marked the PR as draft.

@akantak
Copy link
Author

akantak commented Sep 21, 2023

And the same applies to all calls when using ${0}, like in hammerdbcli, or hammerdb. It should be either changed to ${0##*/} or hardcoded called file name.
These changes are not considered in the PR draft yet.
And, with passing a tcl script with relative path to auto run, changing dir to the HammerDB home, brokes the relative path too.
Let me reconsider that and get back with the solution.

@sm-shaw
Copy link
Contributor

sm-shaw commented Sep 26, 2023

I submitted a draft, but I have still one doubt. in agent/agent line 10, it can be either ${0##*/} so extracting file name only from the path, or hardcoded ./agent like it is in agent/agent.bat. Because when we call, ex. ./agent/agent, but change dir, this path is no longer valid (but used as $0), and should be changed to ./agent. Also, not sure if anybody would start an agent per se, with an added ${HAMMERDB}/agent to the path too. Is it a valid use case @sm-shaw ?

Until we figure this out, I marked the PR as draft.

I think the answer to this is that someone is just as likely to start the agent with an absolute path as much as starting hammerdb, hammerdbcli etc - if we are changing the behaviour then it should be the same for everything. However it all works at the moment using the relative path, this is why we haven't done this before just in case something is inadvertently broken.

@akantak
Copy link
Author

akantak commented Nov 30, 2023

The approach with HAMMERDB_HOME variable has a flaw too, ex. for the hammerdbcli updated with:

#########################################################################
## \
export HAMMERDB_HOME=`dirname -- "$( readlink -f -- "$0"; )";`
## \
export LD_LIBRARY_PATH="${HAMMERDB_HOME}/lib:$LD_LIBRARY_PATH"
## \
export PATH="${HAMMERDB_HOME}/bin:$PATH"
## \
export PYTHONPATH="tclpy0.4:$PYTHONPATH"
## \
exec tclsh8.6 "$0" ${1+"$@"}
########################################################################

it works fine for the binaries used in bash, but in the TCL scripts, there are hardcoded paths to XML files (config/generic.xml) that do not work when called from a directory different than the HammerDB root, so it results with the errors like:

$ hammerdbcli tcl auto example.tcl
HammerDB CLI v4.9
Copyright (C) 2003-2023 Steve Shaw
Type "help" for a list of commands
Could not open XML file config/generic.xml
While loading component file "geninitcli.tcl"...
missing value to go with key
    while executing
"dict append genericdict hdb_version $hdb_version_dict"
    (file "/home/akantak/hammerdb/HammerDB-4.9/src/generic/geninitcli.tcl" line 16)
    invoked from within
"source [ file join $UserDefaultDir src generic $f ]"

Personally, I don't have enough TCL knowledge to tackle that, changing all these relative paths to be based on env variable (?) or variant absolute path.

I also don't want to add another abstraction level to the executables that could work around the "changing directory before call" problem described before. I mean, I would use that locally as a workaround, but won't upstream that.


In our previous discussion, Steve mentioned that there is an issue #383 that potentially can help with that issue, implementing wrapped executables. I think it would be worth waiting for that.

@sm-shaw
Copy link
Contributor

sm-shaw commented Dec 1, 2023

I agree, the potential benefit of this change is outweighed by the amount of testing needed to ensure that everything continues to work.

The preferred solution is to create a single file executable based on a virtual file system that can then be called from anywhere and this is planned in Tcl 8.7 without any external tools https://www.magicsplat.com/blog/tcl87-zipfs2/.

It is also currently possible with Tcl 8.6 https://www.magicsplat.com/blog/starpack-example/ and we already includes Tclkits for the Bawt build process (to bootstrap the build before Tcl is built) and the new CloudTk deployment https://github.com/sm-shaw/CloudTk for running HammerDB as a web application. Therefore, there is a known methodology on how to do this.

We will of course always keep the current packaging as a release where there is preference for direct access to the source in the build, however as Docker is becoming increasingly popular a single file executable is more appropriate for these builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants