From c783e811a43331e4c563438b8fa441792bdcfe28 Mon Sep 17 00:00:00 2001 From: Chris Campbell Date: Fri, 5 Aug 2022 22:43:40 -0700 Subject: [PATCH] fix: correct handling of paths on Windows and those containing spaces (#223) Fixes #222 --- .github/workflows/build.yaml | 29 +++++++++++++++++-- packages/build/src/build/impl/gen-model.ts | 7 +++-- packages/build/src/config/config-loader.ts | 28 +++++++++++++----- packages/cli/package.json | 1 - packages/cli/src/main.js | 7 +++-- packages/cli/src/sde-bundle.js | 4 ++- packages/cli/src/sde-compile.js | 13 +++++---- packages/cli/src/sde-dev.js | 4 ++- packages/cli/src/sde-which.js | 5 +++- packages/cli/src/utils.js | 15 ++++++++++ packages/plugin-check/src/plugin.ts | 26 ++++++++++++----- .../plugin-check/src/vite-config-for-tests.ts | 7 +++-- pnpm-lock.yaml | 24 --------------- scripts/ci-build | 2 +- tests/modeltests | 13 +++++---- 15 files changed, 120 insertions(+), 65 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 81314a68..65f997c6 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -1,5 +1,7 @@ # -# Builds and tests the packages on Linux. +# Builds and tests the packages. This builds on Linux by default, but +# can be configured to build on Windows and/or macOS by uncommenting +# the matrix config lines below. # # This runs after changes are pushed to a feature branch. It does not # run for pushes to the main branch because that is covered by the @@ -15,8 +17,22 @@ on: jobs: build: - runs-on: ubuntu-latest + runs-on: ${{ matrix.config.os }} + timeout-minutes: 20 + strategy: + matrix: + config: + - { plat: 'linux', os: 'ubuntu-20.04' } + # - { plat: 'mac', os: 'macos-10.15' } + # - { plat: 'win', os: 'windows-2019' } steps: + # Force Unix-style line endings (otherwise Prettier checks will fail on Windows) + - name: Configure git to use Unix-style line endings + if: matrix.config.plat == 'win' + run: | + git config --global core.autocrlf false + git config --global core.eol lf + - name: Check out repo uses: actions/checkout@v2 @@ -32,8 +48,15 @@ jobs: with: version: 7 + - name: Configure pnpm + if: matrix.config.plat == 'win' + run: | + # Force pnpm to use bash shell for running scripts on Windows + pnpm config set script-shell bash + - name: Get pnpm store directory id: pnpm-cache + shell: bash run: echo "::set-output name=pnpm_cache_dir::$(pnpm store path)" - name: Enable pnpm cache @@ -45,9 +68,11 @@ jobs: pnpm-store-${{ runner.os }}- - name: Install dependencies + shell: bash run: | pnpm install - name: Build and test + shell: bash run: | ./scripts/ci-build diff --git a/packages/build/src/build/impl/gen-model.ts b/packages/build/src/build/impl/gen-model.ts index 21aebef9..09e01f53 100644 --- a/packages/build/src/build/impl/gen-model.ts +++ b/packages/build/src/build/impl/gen-model.ts @@ -26,9 +26,10 @@ export async function generateModel(context: BuildContext, plugins: Plugin[]): P const config = context.config const prepDir = config.prepDir - // XXX: On Windows, we need to use Windows-specific commands; need to revisit - const isWin = process.platform === 'win32' - const sdeCmdPath = isWin ? `${config.sdeCmdPath}.cmd` : config.sdeCmdPath + // TODO: For now we assume the path is to the `main.js` file in the cli package; + // this seems to work on both Unix and Windows, but we may need to revisit this + // as part of removing the `sdeCmdPath` config hack + const sdeCmdPath = config.sdeCmdPath // Process the mdl file(s) for (const plugin of plugins) { diff --git a/packages/build/src/config/config-loader.ts b/packages/build/src/config/config-loader.ts index 326ec2dc..91e87122 100644 --- a/packages/build/src/config/config-loader.ts +++ b/packages/build/src/config/config-loader.ts @@ -1,8 +1,8 @@ // Copyright (c) 2022 Climate Interactive / New Venture Fund import { existsSync, lstatSync, mkdirSync } from 'fs' -import { join as joinPath, resolve as resolvePath } from 'path' -import { pathToFileURL } from 'url' +import { dirname, join as joinPath, relative, resolve as resolvePath } from 'path' +import { fileURLToPath } from 'url' import type { Result } from 'neverthrow' import { err, ok } from 'neverthrow' @@ -40,10 +40,9 @@ export async function loadConfig( // Use the given `UserConfig` object userConfig = config } else { - // Load the project-specific config. Note that on Windows the import path - // must be a `file://` URL, so we have to convert here. If no `--config` - // arg was specified on the command line, look for a `sde.config.js` file - // in the current directory, and failing that, use a default config. + // Load the project-specific config. If no `--config` arg was specified + // on the command line, look for a `sde.config.js` file in the current + // directory, and failing that, use a default config. // TODO: Create a default config if no file found; for now, we just fail let configPath: string if (typeof config === 'string') { @@ -55,8 +54,8 @@ export async function loadConfig( if (!existsSync(configPath)) { return err(new Error(`Cannot find config file '${configPath}'`)) } - const configUrl = pathToFileURL(configPath).toString() - const configModule = await import(configUrl) + const configRelPath = relativeToSourcePath(configPath) + const configModule = await import(configRelPath) userConfig = await configModule.config() } catch (e) { return err(new Error(`Failed to load config file '${configPath}': ${e.message}`)) @@ -169,3 +168,16 @@ function resolveUserConfig( sdeCmdPath } } + +/** + * Return a Unix-style path (e.g. '../../foo.js') that is relative to the directory of + * the current source file. This can be used to construct a path that is safe for + * dynamic import on either Unix or Windows. + * + * @param filePath The path to make relative. + */ +function relativeToSourcePath(filePath: string): string { + const srcDir = dirname(fileURLToPath(import.meta.url)) + const relPath = relative(srcDir, filePath) + return relPath.replaceAll('\\', '/') +} diff --git a/packages/cli/package.json b/packages/cli/package.json index cbe32d6f..987285cc 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -22,7 +22,6 @@ "@sdeverywhere/compile": "^0.7.0", "bufx": "^1.0.5", "byline": "^5.0.0", - "fs-extra": "^10.1.0", "ramda": "^0.27.0", "shelljs": "^0.8.3", "yargs": "^17.5.1" diff --git a/packages/cli/src/main.js b/packages/cli/src/main.js index 6410b565..ce8bb6cc 100755 --- a/packages/cli/src/main.js +++ b/packages/cli/src/main.js @@ -26,6 +26,7 @@ import fs from 'fs' import path from 'path' import yargs from 'yargs' + import sdeBundle from './sde-bundle.js' import sdeDev from './sde-dev.js' import sdeGenerate from './sde-generate.js' @@ -41,12 +42,14 @@ import sdeTest from './sde-test.js' import sdeNames from './sde-names.js' import sdeCauses from './sde-causes.js' import sdeWhich from './sde-which.js' +import { parentDirForFileUrl } from './utils.js' // Workaround yargs issue where it doesn't find version from package.json // automatically in all cases in ESM context -const srcDir = new URL('.', import.meta.url).pathname +const srcDir = parentDirForFileUrl(import.meta.url) const pkgFile = path.resolve(srcDir, '..', 'package.json') const pkg = JSON.parse(fs.readFileSync(pkgFile)) +const pkgVersion = pkg.version const yarg = yargs(process.argv.slice(2)) yarg @@ -70,7 +73,7 @@ yarg .command(sdeWhich) .demandCommand(1) .help() - .version(pkg.version) + .version(pkgVersion) .alias('h', 'help') .alias('v', 'version') .wrap(yarg.terminalWidth()) diff --git a/packages/cli/src/sde-bundle.js b/packages/cli/src/sde-bundle.js index ccf33cf1..3ea7ee1e 100644 --- a/packages/cli/src/sde-bundle.js +++ b/packages/cli/src/sde-bundle.js @@ -4,6 +4,8 @@ import path from 'path' import { build as runBuild } from '@sdeverywhere/build' +import { parentDirForFileUrl } from './utils.js' + export let command = 'bundle [options]' export let describe = 'build and bundle a model as specified by a config file' export let builder = { @@ -25,7 +27,7 @@ export let bundle = async (configPath, verbose) => { if (verbose) { logLevels.push('verbose') } - const srcDir = new URL('.', import.meta.url).pathname + const srcDir = parentDirForFileUrl(import.meta.url) const sdeDir = path.resolve(srcDir, '..') const sdeCmdPath = path.resolve(srcDir, 'main.js') const result = await runBuild('production', { diff --git a/packages/cli/src/sde-compile.js b/packages/cli/src/sde-compile.js index 498adfa7..71f618b9 100644 --- a/packages/cli/src/sde-compile.js +++ b/packages/cli/src/sde-compile.js @@ -1,8 +1,8 @@ -import fs from 'fs-extra' +import fs from 'fs' import path from 'path' import sh from 'shelljs' -import { buildDir, execCmd, modelPathProps } from './utils.js' +import { buildDir, execCmd, modelPathProps, parentDirForFileUrl } from './utils.js' export let command = 'compile [options] ' export let describe = 'compile the generated model to an executable file' @@ -43,15 +43,16 @@ export default { } let linkCSourceFiles = (modelDirname, buildDirname) => { - let cDirname = path.join(new URL('.', import.meta.url).pathname, 'c') + let srcDir = parentDirForFileUrl(import.meta.url) + let cDirname = path.join(srcDir, 'c') sh.ls(cDirname).forEach(filename => { - // If a C source file is present in the model directory, link to it instead - // as an override. + // If a C source file is present in the model directory, copy that one into + // the build directory to override the one from `src/c`. let srcPathname = path.join(modelDirname, filename) if (!fs.existsSync(srcPathname)) { srcPathname = path.join(cDirname, filename) } let dstPathname = path.join(buildDirname, filename) - fs.ensureSymlinkSync(srcPathname, dstPathname) + fs.copyFileSync(srcPathname, dstPathname) }) } diff --git a/packages/cli/src/sde-dev.js b/packages/cli/src/sde-dev.js index 64602e59..2c731452 100644 --- a/packages/cli/src/sde-dev.js +++ b/packages/cli/src/sde-dev.js @@ -4,6 +4,8 @@ import path from 'path' import { build as runBuild } from '@sdeverywhere/build' +import { parentDirForFileUrl } from './utils.js' + export let command = 'dev [options]' export let describe = 'run a model in a live development environment' export let builder = { @@ -25,7 +27,7 @@ export let dev = async (configPath, verbose) => { if (verbose) { logLevels.push('verbose') } - const srcDir = new URL('.', import.meta.url).pathname + const srcDir = parentDirForFileUrl(import.meta.url) const sdeDir = path.resolve(srcDir, '..') const sdeCmdPath = path.resolve(srcDir, 'main.js') const result = await runBuild('development', { diff --git a/packages/cli/src/sde-which.js b/packages/cli/src/sde-which.js index 43d7a3d7..395edf78 100644 --- a/packages/cli/src/sde-which.js +++ b/packages/cli/src/sde-which.js @@ -1,5 +1,7 @@ import path from 'path' +import { parentDirForFileUrl } from './utils.js' + let command = 'which' let describe = 'print the SDEverywhere home directory' let builder = {} @@ -8,7 +10,8 @@ let handler = argv => { } let which = () => { // The SDEverywhere home directory is one level above the src directory where this code runs. - let homeDir = path.resolve(new URL('..', import.meta.url).pathname) + let srcDir = parentDirForFileUrl(import.meta.url) + let homeDir = path.resolve(srcDir, '..') console.log(homeDir) } export default { diff --git a/packages/cli/src/utils.js b/packages/cli/src/utils.js index 269a6392..36276669 100644 --- a/packages/cli/src/utils.js +++ b/packages/cli/src/utils.js @@ -1,5 +1,6 @@ import fs from 'fs' import path from 'path' +import { fileURLToPath } from 'url' import B from 'bufx' import R from 'ramda' @@ -77,6 +78,20 @@ export function parseSpec(specFilename) { return spec } +/** + * Return the absolute path to the directory in which the given (source) + * file is located. (This is a replacement for `__dirname`, which is not + * available in an ESM context.) + * + * @param srcFileUrl The URL for the source file (i.e., `import.meta.url`). + */ +export function parentDirForFileUrl(srcFileUrl) { + return path.dirname(fileURLToPath(srcFileUrl)) +} + +/** + * Ensure the output directory exists for a given output file path. + */ export function outputDir(outfile, modelDirname) { if (outfile) { outfile = path.dirname(outfile) diff --git a/packages/plugin-check/src/plugin.ts b/packages/plugin-check/src/plugin.ts index 84e872bd..f63cd77e 100644 --- a/packages/plugin-check/src/plugin.ts +++ b/packages/plugin-check/src/plugin.ts @@ -1,7 +1,7 @@ // Copyright (c) 2022 Climate Interactive / New Venture Fund -import { join as joinPath } from 'path' -import { pathToFileURL } from 'url' +import { dirname, join as joinPath, relative } from 'path' +import { fileURLToPath } from 'url' import type { InlineConfig, ViteDevServer } from 'vite' import { build, createServer } from 'vite' @@ -108,9 +108,8 @@ class CheckPlugin implements Plugin { context.log('info', 'Running model checks...') // Load the bundles used by the model check/compare configuration. We - // always initialize the "current" bundle. Note that on Windows the - // dynamic import path must be a `file://` URL, so we have to convert. - const moduleR = await import(pathToFileURL(testOptions.currentBundlePath).toString()) + // always initialize the "current" bundle. + const moduleR = await import(relativeToSourcePath(testOptions.currentBundlePath)) const bundleR = moduleR.createBundle() as Bundle const nameR = testOptions.currentBundleName @@ -121,7 +120,7 @@ class CheckPlugin implements Plugin { let bundleL: Bundle let nameL: string if (this.options?.baseline) { - const moduleL = await import(pathToFileURL(this.options.baseline.path).toString()) + const moduleL = await import(relativeToSourcePath(this.options.baseline.path)) // eslint-disable-next-line @typescript-eslint/no-explicit-any const rawBundleL: any = moduleL.createBundle() as any if (rawBundleL.version === bundleR.version) { @@ -131,7 +130,7 @@ class CheckPlugin implements Plugin { } // Get the model check/compare configuration - const testConfigModule = await import(pathToFileURL(testOptions.testConfigPath).toString()) + const testConfigModule = await import(relativeToSourcePath(testOptions.testConfigPath)) const checkOptions = await testConfigModule.getConfigOptions(bundleL, bundleR, { nameL, nameR @@ -195,3 +194,16 @@ class CheckPlugin implements Plugin { ) } } + +/** + * Return a Unix-style path (e.g. '../../foo.js') that is relative to the directory of + * the current source file. This can be used to construct a path that is safe for + * dynamic import on either Unix or Windows. + * + * @param filePath The path to make relative. + */ +function relativeToSourcePath(filePath: string): string { + const srcDir = dirname(fileURLToPath(import.meta.url)) + const relPath = relative(srcDir, filePath) + return relPath.replaceAll('\\', '/') +} diff --git a/packages/plugin-check/src/vite-config-for-tests.ts b/packages/plugin-check/src/vite-config-for-tests.ts index fda48b28..9daf89ba 100644 --- a/packages/plugin-check/src/vite-config-for-tests.ts +++ b/packages/plugin-check/src/vite-config-for-tests.ts @@ -19,8 +19,11 @@ export function createViteConfigForTests(projDir: string, prepDir: string, mode: // directory where the glob is used). const templateSrcDir = resolvePath(root, 'src') const relProjDir = relative(templateSrcDir, projDir) - // TODO: Use yamlPath from options - const yamlPath = `${relProjDir}/**/*.check.yaml` + // XXX: The glob pattern must use forward slashes only, so on Windows we need to + // convert backslashes to slashes + const relProjDirPath = relProjDir.replaceAll('\\', '/') + // TODO: Use yamlPath from options + const yamlPath = `${relProjDirPath}/**/*.check.yaml` // // Read the `package.json` for the template project // const pkgPath = resolvePath(root, 'package.json') diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index e46fcdb8..b83b9eec 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -171,7 +171,6 @@ importers: '@sdeverywhere/compile': ^0.7.0 bufx: ^1.0.5 byline: ^5.0.0 - fs-extra: ^10.1.0 ramda: ^0.27.0 shelljs: ^0.8.3 yargs: ^17.5.1 @@ -180,7 +179,6 @@ importers: '@sdeverywhere/compile': link:../compile bufx: 1.0.5 byline: 5.0.0 - fs-extra: 10.1.0 ramda: 0.27.2 shelljs: 0.8.5 yargs: 17.5.1 @@ -1645,15 +1643,6 @@ packages: engines: {node: '>=0.8'} dev: false - /fs-extra/10.1.0: - resolution: {integrity: sha512-oRXApq54ETRj4eMiFzGnHWGy+zo5raudjuxN0b8H7s/RU2oW0Wvsx9O0ACRN/kRq9E8Vu/ReskGB5o3ji+FzHQ==} - engines: {node: '>=12'} - dependencies: - graceful-fs: 4.2.10 - jsonfile: 6.1.0 - universalify: 2.0.0 - dev: false - /fs.realpath/1.0.0: resolution: {integrity: sha512-OO0pH2lK6a0hZnAdau5ItzHPI6pUlvI7jMVnxUQRtw4owF2wk8lOSabtGDCTP4Ggrg2MbGnWO9X8K1t4+fGMDw==} @@ -2085,14 +2074,6 @@ packages: resolution: {integrity: sha512-fQzRfAbIBnR0IQvftw9FJveWiHp72Fg20giDrHz6TdfB12UH/uue0D3hm57UB5KgAVuniLMCaS8P1IMj9NR7cA==} dev: true - /jsonfile/6.1.0: - resolution: {integrity: sha512-5dgndWOriYSm5cnYaJNhalLNDKOqFwyDB/rr1E9ZsGciGvKPs8R2xYGCacuf3z6K1YKDz182fd+fY3cn3pMqXQ==} - dependencies: - universalify: 2.0.0 - optionalDependencies: - graceful-fs: 4.2.10 - dev: false - /jstransformer/1.0.0: resolution: {integrity: sha1-7Yvwkh4vPx7U1cGkT2hwntJHIsM=} dependencies: @@ -3404,11 +3385,6 @@ packages: which-boxed-primitive: 1.0.2 dev: true - /universalify/2.0.0: - resolution: {integrity: sha512-hAZsKq7Yy11Zu1DE0OzWjw7nnLZmJZYTDZZyEFHZdUhV8FkH5MCfoU1XMaxXovpyW5nq5scPqq0ZDP9Zyl04oQ==} - engines: {node: '>= 10.0.0'} - dev: false - /uri-js/4.4.1: resolution: {integrity: sha512-7rKUyy33Q1yc98pQ1DAmLtwX109F7TIfWlW1Ydo8Wl1ii1SeHieeh0HHfPeL2fMXK6z0s8ecKs9frCuLJvndBg==} dependencies: diff --git a/scripts/ci-build b/scripts/ci-build index f81a7eb4..b42be195 100755 --- a/scripts/ci-build +++ b/scripts/ci-build @@ -6,7 +6,7 @@ PROJ_DIR=$SCRIPT_DIR/.. set -e # fail on error set -x # include all commands in logs -cd $PROJ_DIR +cd "$PROJ_DIR" pnpm run prettier-local:check pnpm run -r --workspace-concurrency=1 ci:build diff --git a/tests/modeltests b/tests/modeltests index 7a3c1ff7..52cc6d7b 100755 --- a/tests/modeltests +++ b/tests/modeltests @@ -5,11 +5,12 @@ PROJ_DIR=$SCRIPT_DIR/.. MODELS_DIR=$PROJ_DIR/models set -e # fail on error +#set -x -cd $PROJ_DIR +cd "$PROJ_DIR" # Use the local sde command. -SDE="node $PROJ_DIR/packages/cli/src/main.js" +SDE_MAIN="$PROJ_DIR/packages/cli/src/main.js" function test { MODEL=$1 @@ -17,7 +18,7 @@ function test { MODEL_DIR=$MODELS_DIR/$MODEL # Clean up before - $SDE clean --modeldir $MODEL_DIR + node "$SDE_MAIN" clean --modeldir "$MODEL_DIR" # Test (only if there is a dat file to compare against) if [[ -f $MODEL_DIR/${MODEL}.dat ]]; then @@ -32,7 +33,7 @@ function test { if [[ $MODEL == "allocate" ]]; then PRECISION="1e-2" fi - $SDE test $TEST_ARGS -p $PRECISION $MODEL_DIR/$MODEL + node "$SDE_MAIN" test $TEST_ARGS -p $PRECISION "$MODEL_DIR/$MODEL" fi # Run additional script to validate output @@ -43,7 +44,7 @@ function test { fi # Clean up after - $SDE clean --modeldir $MODEL_DIR + node "$SDE_MAIN" clean --modeldir "$MODEL_DIR" echo } @@ -53,7 +54,7 @@ if [[ -n $1 ]]; then test $1 else # Test each model against saved Vensim data. - for m in $(ls $MODELS_DIR); do + for m in $(ls "$MODELS_DIR"); do test $m done fi