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

fix: resolve nushell support issue #397

Merged
merged 2 commits into from
Jan 26, 2025
Merged

fix: resolve nushell support issue #397

merged 2 commits into from
Jan 26, 2025

Conversation

bytemain
Copy link
Member

No description provided.

Copy link

codecov bot commented Jan 25, 2025

Codecov Report

Attention: Patch coverage is 0% with 91 lines in your changes missing coverage. Please review.

Project coverage is 28.26%. Comparing base (f14428d) to head (584b0e9).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/manager.go 0.00% 73 Missing ⚠️
internal/shell/nushell.go 0.00% 13 Missing ⚠️
internal/shell/bash.go 0.00% 1 Missing ⚠️
internal/shell/clink.go 0.00% 1 Missing ⚠️
internal/shell/fish.go 0.00% 1 Missing ⚠️
internal/shell/powershell.go 0.00% 1 Missing ⚠️
internal/shell/zsh.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #397      +/-   ##
==========================================
- Coverage   28.89%   28.26%   -0.63%     
==========================================
  Files          42       42              
  Lines        4122     4206      +84     
==========================================
- Hits         1191     1189       -2     
- Misses       2806     2892      +86     
  Partials      125      125              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bytemain
Copy link
Member Author

@colececil I didn't have any windows machine, could you test it in windows? it actually worked in mac:
image

@bytemain bytemain merged commit 7db98f3 into main Jan 26, 2025
3 of 4 checks passed
@colececil
Copy link
Contributor

@bytemain Hmm, it still doesn't seem to be working for me in Windows. Here are the steps I followed:

  1. Pulled the latest from main in the vfox repo, so I have your changes.
  2. Ran go build . in the project root, to create vfox.exe at the project root.
  3. Ran ./vfox.exe activate nushell $nu.default-config-dir | save --append $nu.config-path from the project root, to create the vfox.nu config script and to source it from the Nushell config file. (By the way, I like your idea to move the vfox script to its own file to keep the Nushell config file cleaner and to not have to worry about using the right line endings. Nice work!)
  4. Restarted my shell, so the config changes take effect.

After doing that, here's what I'm getting:

~\repos\external-projects\vfox> open $nu.config-path | lines | last
source ($nu.default-config-dir | path join "vfox.nu")
~\repos\external-projects\vfox> open $'($nu.default-config-dir)/vfox.nu'

# vfox configuration
# this make sure this configuration is up to date when you open a new shell
^'C:/Users/colec/repos/external-projects/vfox/vfox.exe' activate nushell $nu.default-config-dir | ignore

export-env {
  def --env updateVfoxEnvironment [] {
    let envData = (^'C:/Users/colec/repos/external-projects/vfox/vfox.exe' env -s nushell --full | from json)
    load-env $envData.envsToSet
    hide-env ...$envData.envsToUnset
  }
  $env.config = ($env.config | upsert hooks.pre_prompt {
    let currentValue = ($env.config | get -i hooks.pre_prompt)
    if $currentValue == null {
      [{updateVfoxEnvironment}]
    } else {
      $currentValue | append {updateVfoxEnvironment}
    }
  })
  $env.__VFOX_SHELL = 'nushell'
  $env.__VFOX_PID = $nu.pid
  ^'C:/Users/colec/repos/external-projects/vfox/vfox.exe' env --cleanup | ignore
  updateVfoxEnvironment
}
~\repos\external-projects\vfox> ./vfox.exe use nodejs
Please select a version of nodejs:
  -> 22.11.0
Now using [email protected].
~\repos\external-projects\vfox> node --version
v20.14.0
~\repos\external-projects\vfox> which node
╭───┬─────────┬───────────────────────────────────────────────────────────┬──────────╮
│ # │ command │                           path                            │   type   │
├───┼─────────┼───────────────────────────────────────────────────────────┼──────────┤
│ 0 │ node    │ C:\Users\colec\.version-fox\cache\nodejs\current\node.exe │ external │
╰───┴─────────┴───────────────────────────────────────────────────────────┴──────────╯

All the config stuff there looks right to me, but it's not using the nodejs version I told it to with vfox use nodejs. Let me know if there's anything else I can do to help debug the issue here. Thanks!

@colececil
Copy link
Contributor

Ok, here's some more info to go off of for debugging:

~\repos\external-projects\vfox> ./vfox.exe use nodejs
Please select a version of nodejs:
  -> 22.11.0
Now using [email protected].
~\repos\external-projects\vfox> ./vfox.exe env -s nushell --full
{"envsToSet":{"GOPATH":"C:\\Users\\colec\\.version-fox\\temp\\1738044000-16820\\golang/packages","GOROOT":"C:\\Users\\colec\\.version-fox\\temp\\1738044000-16820\\golang","JAVA_HOME":"C:\\Users\\colec\\.version-fox\\temp\\1738044000-16820\\java","PATH":["C:\\Users\\colec\\.version-fox\\temp\\1738044000-16820\\nodejs","C:\\Users\\colec\\.version-fox\\temp\\1738044000-16820\\golang\\bin","C:\\Users\\colec\\.version-fox\\temp\\1738044000-16820\\golang\\packages\\bin","C:\\Users\\colec\\.version-fox\\temp\\1738044000-16820\\java\\bin","C:\\Program Files\\BOSSA","C:\\Program Files (x86)\\Intel\\iCLS Client\\","C:\\Program Files\\Intel\\iCLS Client\\","C:\\Windows\\system32","C:\\Windows","C:\\Windows\\System32\\Wbem","C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\","C:\\Program Files (x86)\\Intel\\Intel(R) Management Engine Components\\DAL","C:\\Program Files\\Intel\\Intel(R) Management Engine Components\\DAL","C:\\Program Files (x86)\\Intel\\Intel(R) Management Engine Components\\IPT","C:\\Program Files\\Intel\\Intel(R) Management Engine Components\\IPT","C:\\Program Files (x86)\\NVIDIA Corporation\\PhysX\\Common","C:\\ProgramData\\chocolatey\\bin","C:\\Program Files (x86)\\GtkSharp\\2.12\\bin","C:\\WINDOWS\\system32","C:\\WINDOWS","C:\\WINDOWS\\System32\\Wbem","C:\\WINDOWS\\System32\\WindowsPowerShell\\v1.0\\","C:\\WINDOWS\\System32\\OpenSSH\\","C:\\Program Files\\Docker\\Docker\\resources\\bin","C:\\Program Files\\NVIDIA Corporation\\NVIDIA app\\NvDLISR","C:\\Program Files\\dotnet\\","C:\\Users\\colec\\.version-fox\\cache\\golang\\current\\packages\\bin","C:\\Users\\colec\\.version-fox\\cache\\golang\\current\\bin","C:\\Users\\colec\\.version-fox\\cache\\nodejs\\current","C:\\Users\\colec\\.version-fox\\cache\\java\\current\\bin","C:\\Users\\colec\\scoop\\apps\\vscodium\\current\\bin","C:\\Users\\colec\\scoop\\shims","C:\\Ruby22-x64\\bin","C:\\Users\\colec\\AppData\\Local\\Microsoft\\WindowsApps","C:\\Program Files\\Microsoft VS Code\\bin","C:\\tools\\cmder","C:\\Program Files\\Aseprite","C:\\Users\\colec\\AppData\\Local\\JetBrains\\Toolbox\\scripts","C:\\Users\\colec\\repos\\familiar.sh"]},"envsToUnset":[]}
~\repos\external-projects\vfox> $env.PATH | first 5
╭───┬───────────────────────────────────────────────────────────────────────╮
│ 0 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs              │
│ 1 │ C:\Users\colec\.version-fox\temp\1738044000-16820\golang\bin          │
│ 2 │ C:\Users\colec\.version-fox\temp\1738044000-16820\golang\packages\bin │
│ 3 │ C:\Users\colec\.version-fox\temp\1738044000-16820\java\bin            │
│ 4 │ C:\Program Files\BOSSA                                                │
╰───┴───────────────────────────────────────────────────────────────────────╯
~\repos\external-projects\vfox> node --version
v20.14.0
~\repos\external-projects\vfox> which node
╭───┬─────────┬───────────────────────────────────────────────────────────┬──────────╮
│ # │ command │                           path                            │   type   │
├───┼─────────┼───────────────────────────────────────────────────────────┼──────────┤
│ 0 │ node    │ C:\Users\colec\.version-fox\cache\nodejs\current\node.exe │ external │
╰───┴─────────┴───────────────────────────────────────────────────────────┴──────────╯
~\repos\external-projects\vfox> ls C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs
╭────┬────────────────────────────────────────────────────────────────────────────┬──────┬───────────┬──────────────╮
│  # │                                    name                                    │ type │   size    │   modified   │
├────┼────────────────────────────────────────────────────────────────────────────┼──────┼───────────┼──────────────┤
│  0 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\CHANGELOG.md      │ file │  53.9 KiB │ 8 months ago │
│  1 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\LICENSE           │ file │ 116.8 KiB │ 8 months ago │
│  2 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\README.md         │ file │  38.6 KiB │ 8 months ago │
│  3 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\corepack          │ file │     334 B │ 8 months ago │
│  4 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\corepack.cmd      │ file │     218 B │ 8 months ago │
│  5 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\install_tools.bat │ file │   3.0 KiB │ 8 months ago │
│  6 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\node.exe          │ file │  66.1 MiB │ 8 months ago │
│  7 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\node_modules      │ dir  │       0 B │ 8 months ago │
│  8 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\nodevars.bat      │ file │     702 B │ 8 months ago │
│  9 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\npm               │ file │   2.0 KiB │ 8 months ago │
│ 10 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\npm.cmd           │ file │     538 B │ 8 months ago │
│ 11 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\npx               │ file │   2.0 KiB │ 8 months ago │
│ 12 │ C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\npx.cmd           │ file │     538 B │ 8 months ago │
╰────┴────────────────────────────────────────────────────────────────────────────┴──────┴───────────┴──────────────╯
~\repos\external-projects\vfox> C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs\node.exe --version
v20.14.0

There are two things I'm not understanding:

  1. Since C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs is before C:\Users\colec\.version-fox\cache\nodejs\current in the PATH, why does which node return C:\Users\colec\.version-fox\cache\nodejs\current?
  2. The version of node in C:\Users\colec\.version-fox\temp\1738044000-16820\nodejs seems to be v20.14.0. Why would vfox be putting this directory at the top of the PATH since I told it I wanted to use node v22.11.0?

@colececil
Copy link
Contributor

I also reviewed your code changes in this pull request, and they made sense to me.

The only thing I'm not understanding is what's in manager.go, because that part of the codebase is a bit unclear to me. I was having a hard time understanding that part when I was working on my changes too. I know that's what determines what environment variables to return, but I don't understand how it works. It seems this may be where the issue lies, given number 2 in my previous comment above?

@bytemain
Copy link
Member Author

I also reviewed your code changes in this pull request, and they made sense to me.

The only thing I'm not understanding is what's in manager.go, because that part of the codebase is a bit unclear to me. I was having a hard time understanding that part when I was working on my changes too. I know that's what determines what environment variables to return, but I don't understand how it works. It seems this may be where the issue lies, given number 2 in my previous comment above?

I'm currently on Chinese New Year holiday. I'll look into the bug as soon as I return.

@colececil
Copy link
Contributor

Enjoy! 🎉

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

Successfully merging this pull request may close these issues.

2 participants