From e09a00189e0406b4c45a0c393cb0048c55306f23 Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Thu, 2 Feb 2023 15:26:49 -0500 Subject: [PATCH 1/3] init: make compatible with Windows On Windows, 'bbi init --dir X' doesn't write "nonmem" entries to bbi.yaml even if X is valid. There are two issues: * isPathNonMemmy() and findNonMemBinary() assume that a nmfe binary will have an executable bit, but that isn't the case on Windows * findNonMemBinary() doesn't account for the nmfe binary ending with ".bat" on Windows Fix isPathNonMemmy() by skipping the executable bit check on Windows. For findNonMemBinary(), add a new function. Another option would be to update findNonMemBinary() with conditional logic but 1) it's nice to have a separate function we can test when not on Windows (especially given we don't have Windows CI right now) and 2) the duplication isn't too bad because the remaining shared logic is mostly just listing a directory. Fixes #295. --- cmd/init.go | 45 ++++++++++++++++++++++++++++++++++++++++++++- cmd/init_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 cmd/init_test.go diff --git a/cmd/init.go b/cmd/init.go index e6f9c41f..21fe3fd8 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "regexp" + "runtime" "strings" "github.com/metrumresearchgroup/bbi/configlib" @@ -28,6 +29,13 @@ func initializer(cmd *cobra.Command, _ []string) error { return fmt.Errorf("get dir string: %w", err) } + var find_nm func(string) (string, error) + if runtime.GOOS == "windows" { + find_nm = findNonMemBinaryWindows + } else { + find_nm = findNonMemBinary + } + for _, l := range dir { var files []os.FileInfo // For each directory underneath the dir provided. Let's see if it's nonmemmy @@ -51,7 +59,7 @@ func initializer(cmd *cobra.Command, _ []string) error { for _, v := range locations { var nm string - nm, err = findNonMemBinary(v) + nm, err = find_nm(v) if err != nil { log.Println(err) @@ -133,6 +141,12 @@ func isPathNonMemmy(path string) bool { return false } + // That rest of this function (checking for an executable bit in + // file mode bits) isn't relevant for Windows. + if runtime.GOOS == "windows" { + return true + } + // Are any of them executable? fails := 0 @@ -188,6 +202,35 @@ func findNonMemBinary(path string) (string, error) { return "", errors.New("No nonmem binary could be located in the given path. Please check again or try another directory") } +// findNonMemBinaryWindows is a Windows-specific variant of +// findNonMemBinary. The key differences are that 1) it expects +// ".bat" at the end of the binary and 2) it doesn't expect an +// executable bit to be set. +func findNonMemBinaryWindows(path string) (string, error) { + rdir := filepath.Join(path, "run") + fd, err := os.Open(rdir) + if err != nil { + return "", err + } + defer fd.Close() + + re := regexp.MustCompile(`^nmfe[0-9]{2}\.bat$`) + files, err := fd.Readdirnames(-1) + if err != nil { + return "", err + } + + for _, fname := range files { + // Follow findNonMemBinary() and take the first match (rather + // than, e.g., returning an error if multiple files match). + if re.MatchString(fname) { + return fname, nil + } + } + + return "", fmt.Errorf("nmfe .bat file not found in %s", rdir) +} + func hasNMQual(path string) bool { fs := afero.NewOsFs() diff --git a/cmd/init_test.go b/cmd/init_test.go new file mode 100644 index 00000000..469fe5c6 --- /dev/null +++ b/cmd/init_test.go @@ -0,0 +1,35 @@ +package cmd + +import ( + "os" + "path/filepath" + "testing" + + "github.com/metrumresearchgroup/wrapt" +) + +func TestFindNonMemBinaryWindows(tt *testing.T) { + t := wrapt.WrapT(tt) + dir := t.TempDir() + rdir := filepath.Join(dir, "run") + if err := os.Mkdir(rdir, 0777); err != nil { + t.Fatal(err) + } + + foo := filepath.Join(rdir, "foo.bat") + if err := os.WriteFile(foo, []byte(""), 0666); err != nil { + t.Fatal(err) + } + + _, err := findNonMemBinaryWindows(dir) + t.R.Contains(err.Error(), ".bat file not found") + + nmfe := filepath.Join(rdir, "nmfe23.bat") + if err = os.WriteFile(nmfe, []byte(""), 0666); err != nil { + t.Fatal(err) + } + + binary, err := findNonMemBinaryWindows(dir) + t.R.NoError(err) + t.R.Equal(binary, "nmfe23.bat") +} From 1613aa553c10309a2b1413b5d4fc24bdd99f2373 Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Thu, 2 Feb 2023 15:26:54 -0500 Subject: [PATCH 2/3] run local: invoke bash directly rather than relying on shebangs When `bbi nonmem run local ...` is called, a NONMEM command is written to `{model}.sh`, and the script is passed as the first argument to exec.Command(), relying on the presence of a /bin/bash shebang. Likewise, if --post_work_executable is specified, the post_processing.sh path is passed as the first argument. On Metworx (and most Linux systems), hard coding /bin/bash works fine, but it will fail on systems that happen to have bash at a different location. To avoid hard coding the path, we could find the bash executable and write the appropriate shebang. However, that won't work for Windows, which doesn't support shebangs by default. So instead pass bash as the main program to exec.Command(), feeding the script as the argument. This still won't work _by default_ on Windows systems. But * there's now a clearer failure saying that bash is required * it is relatively straightforward for users to install bash via either Rtools or Git for Windows (and many users will already have one of these available). And then in the context of R/bbr, users will need to adjust their .Renviron with an entry like PATH="${RTOOLS40_HOME}\usr\bin;${PATH}" # Or # PATH="C:\Program Files\Git\bin;${PATH}" Note the shebangs are still included in the scripts and the scripts are still executable, so if people were used to being able to debug by going to the model directory and running `./x.sh`, they can still do so. Also, note that executing the SGE grid.sh script still relies on the hard-coded /bin/bash shebang. (We could adjust the qsub invocation to avoid that, but it's not likely to matter in practice.) --- cmd/local.go | 11 ++++++++++- cmd/run.go | 7 ++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/cmd/local.go b/cmd/local.go index 00b340e5..73c018d7 100644 --- a/cmd/local.go +++ b/cmd/local.go @@ -443,7 +443,16 @@ func executeLocalJob(model *NonMemModel) turnstile.ConcurrentError { log.Debugf("Script location is pegged at %s", scriptLocation) - command := exec.Command(scriptLocation) + bash, err := exec.LookPath("bash") + if err != nil { + return turnstile.ConcurrentError{ + Error: err, + RunIdentifier: model.FileName, + Notes: fmt.Sprintf("bash is required: %v", err), + } + } + + command := exec.Command(bash, scriptLocation) command.Dir = model.OutputDir command.Env = os.Environ() // Take in OS Environment diff --git a/cmd/run.go b/cmd/run.go index e3b60ffe..d857d1f7 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -287,8 +287,13 @@ func ExecutePostWorkDirectivesWithEnvironment(worker PostWorkExecutor) (string, return "", err } + bash, err := exec.LookPath("bash") + if err != nil { + return "", err + } + // Needs to be the processed value, not the config template. - cmd := exec.Command(filepath.Join(worker.GetWorkingPath(), "post_processing.sh")) + cmd := exec.Command(bash, filepath.Join(worker.GetWorkingPath(), "post_processing.sh")) // Set the environment for the binary. cmd.Env = environment From 60dc21b4fd41395db40e89d0fd61a9a83346b04d Mon Sep 17 00:00:00 2001 From: Kyle Meyer Date: Fri, 3 Feb 2023 14:13:20 -0500 Subject: [PATCH 3/3] run local: make "bash not found" message more helpful Re: https://github.com/metrumresearchgroup/bbi/pull/296#discussion_r1095987690 --- cmd/local.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/local.go b/cmd/local.go index 73c018d7..81353089 100644 --- a/cmd/local.go +++ b/cmd/local.go @@ -448,7 +448,7 @@ func executeLocalJob(model *NonMemModel) turnstile.ConcurrentError { return turnstile.ConcurrentError{ Error: err, RunIdentifier: model.FileName, - Notes: fmt.Sprintf("bash is required: %v", err), + Notes: "bash is required to run bbi. Please install bash and then try running this again.", } }