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

sde command fails on Windows and/or when path contains spaces #222

Closed
chrispcampbell opened this issue Aug 3, 2022 · 2 comments · Fixed by #223 or #224
Closed

sde command fails on Windows and/or when path contains spaces #222

chrispcampbell opened this issue Aug 3, 2022 · 2 comments · Fixed by #223 or #224
Assignees

Comments

@chrispcampbell
Copy link
Contributor

The sde CLI fails to run on Windows due to a late change I made before publishing 0.7.0:

$ sde bundle --config dist/sde-config.js --verbose
node:fs:585
  handleErrorFromBinding(ctx);
  ^

Error: ENOENT: no such file or directory, open 'D:\D:\a\c-roads-app\c-roads-app\node_modules\@sdeverywhere\cli\package.json'
    at Object.openSync (node:fs:585:3)
    at Object.readFileSync (node:fs:453:35)
    at file:///D:/a/c-roads-app/c-roads-app/node_modules/@sdeverywhere/cli/src/main.js:49:27
    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:385:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
    at async handleMainPromise (node:internal/modules/run_main:61:12) {
  errno: -4058,
  syscall: 'open',
  code: 'ENOENT',
  path: 'D:\\D:\\a\\c-roads-app\\c-roads-app\\node_modules\\@sdeverywhere\\cli\\package.json'
}

The code that loads the current version from package.json needs to be updated to be more Windows friendly.

As part of this, we should extend the CI build script to (optionally) build on Windows in addition to Linux to catch these issues earlier.

@chrispcampbell chrispcampbell self-assigned this Aug 3, 2022
@chrispcampbell chrispcampbell changed the title sde command fails on Windows sde command fails on Windows and/or when path contains spaces Aug 5, 2022
@chrispcampbell
Copy link
Contributor Author

The specific cause of this particular issue is related to the way we get the directory name for the current source file. I think before we converted the sources to be ESM-friendly, we'd just use __dirname, but the ESM replacement used in a few places was this:

  const srcDir = new URL('.', import.meta.url).pathname

In other projects, I used the following:

  const srcDir = dirname(fileURLToPath(import.meta.url))

On Unix, these two forms produce similar/compatible results:

/Users/campbell/Projects/CI/sdeverywhere/packages/cli/src/
/Users/campbell/Projects/CI/sdeverywhere/packages/cli/src

But on Windows, they produce different results:

/C:/Users/Chris%20Campbell/win/SDEverywhere/packages/cli/src/
C:\Users\Chris Campbell\win\SDEverywhere\packages\cli\src

Additionally, the first form doesn't lead to usable paths on Unix when there are spaces in the directory name:

/Users/campbell/Projects/CI/sdeverywhere%20with%20spaces/packages/cli/src/
/Users/campbell/Projects/CI/sdeverywhere with spaces/packages/cli/src

To fix this particular issue, I think we should switch to the second form. I had started working on additional changes to allow for building/testing on Windows in our GitHub Actions workflow, but there are likely other issues to resolve, so I will file a separate issue for that work.

@chrispcampbell
Copy link
Contributor Author

There were a couple of other related issues that I fixed as part of this:

  1. When loading config files via dynamic import, we were converting paths to file: URLs because Node requires that when passing absolute paths on Windows. However, this approach didn't work with Vitest and caused a test to fail. A more general approach (that works on Windows and Unix) is to convert the path to a relative one (and use forward slashes only), so I switched to that.

  2. There was one place in sde-compile.js that used fs-extra to symlink files from the cli/src/c directory. This symlinking approach doesn't generally work (without extra configuration) on Windows, and it doesn't seem critical to symlink there, so I changed it to copy the files instead. This was the last usage of fs-extra so I removed it as a dependency.

  3. A couple places in build/test scripts would fail if run from a path that includes spaces, so I added quotes where necessary.

  4. I left the changes to build.yaml that allow for building on Windows/macOS intact, but commented out those parts of the matrix so that we still only build on Linux by default. I think after all the above changes, the build and modeltests almost all work on Windows, except for a couple header related issues (for example, M_PI undefined), so I will investigate those later under a separate issue. The main takeaway for this issue is that the Windows build now gets much farther along than it did prior to these changes (the build package tests now complete, and the hello-world example works on Windows even from a path that includes spaces).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant