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

Add missing dependencies entry for which, which was causing the wrong version of which to be used, which caused R not to load on some configurations of Windows #4311

Merged

Conversation

softwarenerd
Copy link
Contributor

@softwarenerd softwarenerd commented Aug 10, 2024

This PR addresses #4280.

This issue was tracked down to the fact that the positron-r extension did not have an explicit dependencies entry for the which package, but it did have a devDependencies entry for @types/which ("@types/which": "^3.0.0",).

As a result of this, the positron-r/node_modules folder contained which 2.0.2 (I believe) from:

mocha@^9.2.1:
  version "9.2.2"
  resolved "https://registry.npmjs.org/mocha/-/mocha-9.2.2.tgz"
  integrity sha512-L6XC3EdwT6YrIk0yXpavvLkn8h+EU+Y5UcCHKECyMbdUIxyMuZj4bX4U9e1nvnvUUvQVsV2VHQr5zLdcUkhW/g==
  dependencies:
    "@ungap/promise-all-settled" "1.1.2"
    ansi-colors "4.1.1"
    browser-stdout "1.3.1"
    chokidar "3.5.3"
    debug "4.3.3"
    diff "5.0.0"
    escape-string-regexp "4.0.0"
    find-up "5.0.0"
    glob "7.2.0"
    growl "1.10.5"
    he "1.2.0"
    js-yaml "4.1.0"
    log-symbols "4.1.0"
    minimatch "4.2.1"
    ms "2.1.3"
    nanoid "3.3.1"
    serialize-javascript "6.0.0"
    strip-json-comments "3.1.1"
    supports-color "8.1.1"
    which "2.0.2"
    workerpool "6.2.0"
    yargs "16.2.0"
    yargs-parser "20.2.4"
    yargs-unparser "2.0.0"

This version of which does not support the nothrow option for async which, which we rely on in this code fragment from extensions/positron-r/src/provider.ts:

	const whichR = await which('R', { nothrow: true }) as string;

Because of this, on some configurations of Windows, an error was being thrown which caused R not to load.

For background on this, see: npm/node-which#80. This was the exact same problem.

QA Notes

@softwarenerd softwarenerd changed the title Add missing dependencies entry for which Add missing dependencies entry for which which was causing the wrong version of which to be used which caused R not to load on Windows Aug 10, 2024
@softwarenerd softwarenerd changed the title Add missing dependencies entry for which which was causing the wrong version of which to be used which caused R not to load on Windows Add missing dependencies entry for which which was causing the wrong version of which to be used which caused R not to load on Windows Aug 10, 2024
@softwarenerd softwarenerd changed the title Add missing dependencies entry for which which was causing the wrong version of which to be used which caused R not to load on Windows Add missing dependencies entry for which, which was causing the wrong version of which to be used, which caused R not to load on Windows Aug 10, 2024
@softwarenerd softwarenerd changed the title Add missing dependencies entry for which, which was causing the wrong version of which to be used, which caused R not to load on Windows Add missing dependencies entry for which, which was causing the wrong version of which to be used, which caused R not to load on some configurations of Windows Aug 10, 2024
jennybc
jennybc previously approved these changes Aug 10, 2024
Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So happy we ran this down!

Works fine for me in my Windows VM (where I could never reproduce this), so that's good.

And it just plain makes sense.

Comment on lines 601 to 602
"winreg": "^1.2.5",
"which": "^3.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it feels like we're keeping these in alphabetical order? In which case we should:

Suggested change
"winreg": "^1.2.5",
"which": "^3.0.0"
"which": "^3.0.0",
"winreg": "^1.2.5"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot on. Fixed.

@@ -2315,6 +2315,13 @@ which@2.0.2, which@^2.0.1:
dependencies:
isexe "^2.0.0"

which@^3.0.0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@softwarenerd softwarenerd merged commit 7ee0596 into main Aug 10, 2024
2 checks passed
@softwarenerd softwarenerd deleted the fix/fix-for-r-not-being-found-at-startup-on-windows branch August 10, 2024 04:38
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants