FOEDAG icon indicating copy to clipboard operation
FOEDAG copied to clipboard

Windows: LocateExecFile() fails to find python3

Open skyler-rs opened this issue 2 years ago • 2 comments

Describe the bug When adding a new test, it was discovered that system calls to python in the IP Catalog were failing. After some debug, it was observed that https://github.com/os-fpga/FOEDAG/blob/809bd94533e4e0e47db656249b6c0ba6c279f7e2/src/IPGenerate/IPCatalogBuilder.cpp#L120 is always empty in CI tests run on windows. On first glance the 2 possible causes are:

  1. Python might not be properly installed on windows in our CI
  2. LocateExecFile might not be working on windows

To Reproduce Due to windows build not building locally for some right now, this is a bit hard to test, I'm unable to build windows locally so I can only see this error in github CI's when the tests are run on windows (note the test in question is currently disabled on windows). Steps to reproduce the behavior:

  1. Add the following to the bottom of registerBasicGuiCommands()
  auto locate_exec_file = [](void* clientData, Tcl_Interp* interp, int argc,
                             const char* argv[]) -> int {
    if (argc < 1) {
      Tcl_AppendResult(
          interp, qPrintable("Expected Syntax: locate_exec_file <binary_name>"),
          nullptr);
      return TCL_ERROR;
    }

    std::filesystem::path foundPath =
        FOEDAG::FileUtils::LocateExecFile(argv[1]);

    Tcl_AppendResult(interp, foundPath.c_str(), nullptr);
    return TCL_OK;
  };

  session->TclInterp()->registerCmd("locate_exec_file", locate_exec_file,
                                    GlobalSession->MainWindow(), nullptr);
  1. Add #include "Utils/FileUtils.h" to the same file
  2. Build changes
  3. On windows, open foedag and try locate_exec_file python3 in the console

Expected behavior

  • FileUtils::LocateExecFile("python3") or test tcl command locate_exec_file python3 should return a valid path on windows runs
  • tests/TestBatch/test_ip_configure_load.tcl should successfully run in windows CI (note the test currently disables itself on windows run, so remove the windows check inside the test first before verifying)

Enviornment (please complete the following information):

  • OS: Other: Windows GitHub CI and possibly local windows
  • Version: Current main

Additional context It's currently not known if this is a python install issue or a util function not working in windows issue so both areas need to be investigated It's possible that python3 isn't the proper name on windows, might test with python and python2 to see if our target binary names are correct

skyler-rs avatar Sep 19 '22 21:09 skyler-rs

LocateExecFile needs to be augmented for Windows support, it is only working for Linux. It needs to be augmented to search the Windows PATH and system install directories similarly to Linux.

std::filesystem::path FileUtils::LocateExecFile(
    const std::filesystem::path& path) {
  std::filesystem::path result;
  char* envpath = getenv("PATH");
  char* dir = nullptr;

  for (dir = strtok(envpath, ":"); dir; dir = strtok(NULL, ":")) {
    std::filesystem::path a_path = std::string(dir) / path;
    if (FileUtils::FileExists(a_path)) {
      return a_path;
    }
  }

  for (std::filesystem::path dir :
       {"/usr/bin", "/usr/local/bin", "~/.local/bin", "./"}) {
    std::filesystem::path a_path = dir / path;
    if (FileUtils::FileExists(a_path)) {
      return a_path;
    }
  }

  return result;
}

alaindargelas avatar Sep 19 '22 21:09 alaindargelas

LocateExecFile needs to be augmented for Windows support, it is only working for Linux. It needs to be augmented to search the Windows PATH and system install directories similarly to Linux.

std::filesystem::path FileUtils::LocateExecFile(
    const std::filesystem::path& path) {
  std::filesystem::path result;
  char* envpath = getenv("PATH");
  char* dir = nullptr;

  for (dir = strtok(envpath, ":"); dir; dir = strtok(NULL, ":")) {
    std::filesystem::path a_path = std::string(dir) / path;
    if (FileUtils::FileExists(a_path)) {
      return a_path;
    }
  }

  for (std::filesystem::path dir :
       {"/usr/bin", "/usr/local/bin", "~/.local/bin", "./"}) {
    std::filesystem::path a_path = dir / path;
    if (FileUtils::FileExists(a_path)) {
      return a_path;
    }
  }

  return result;
}

This makes sense now, I see that we will have to handle python path look up in mac/centos too.

ravic-rs avatar Sep 20 '22 06:09 ravic-rs