janet-lang / jpm

Janet Project Manager
MIT License
68 stars 22 forks source link

jpm deps behavior on Windows #10

Closed sogaiu closed 3 years ago

sogaiu commented 3 years ago

With cf6086ae3b282046141fd7b6fbd1d08edcb85fdd on Windows, jpm deps for a project with at least one dependency can give output like this:

C:\Users\user\Desktop\jpm-sample>jpm deps
From https://github.com/janet-lang/spork
 * branch            master     -> FETCH_HEAD
Already up to date.
HEAD is now at 98f61de Merge pull request #44 from subsetpark/streams
C:/Users/user/scoop/apps/git/2.24.1.windows.2/mingw64/libexec/git-core\git-submodule: line 7: sed: command not found
C:/Users/user/scoop/apps/git/2.24.1.windows.2/mingw64/libexec/git-core\git-submodule: line 21: .: git-sh-setup: file not found
error: command failed with non-zero exit code 1
  in os/execute [src/core/os.c] on line 1049
  in shell [C:/Users/user/AppData/Local/Apps/Janet/Library/jpm/shutil.janet] (tailcall) on line 107, column 5
  in download-bundle [C:/Users/user/AppData/Local/Apps/Janet/Library/jpm/pm.janet] on line 167, column 10
  in bundle-install [C:/Users/user/AppData/Local/Apps/Janet/Library/jpm/pm.janet] on line 179, column 13
  in deps [C:/Users/user/AppData/Local/Apps/Janet/Library/jpm/commands.janet] (tailcall) on line 123, column 7
  in _thunk [C:\Users\user\AppData\Local\Apps\Janet\bin\/jpm] on line -1, column -1
  in cli-main [boot.janet] on line 3618, column 39

Following a lead from bakpakin, I tried disabling a part of this line: https://github.com/janet-lang/jpm/blob/cf6086ae3b282046141fd7b6fbd1d08edcb85fdd/jpm/shutil.janet#L101

A subsequent execution of jpm deps completed successfully.

Version info of various bits:

bakpakin commented 3 years ago

Should be fixed in 588485be0c834d39ade9a967f254f9ca3a373082

sogaiu commented 3 years ago

I tried with 588485be0c834d39ade9a967f254f9ca3a373082 and 996a59e28ff758de5b73822dafa8e343e9879958 but still get similar output.

Before trying I tried to remove jpm via jpm uninstall from a local source repository for jpm and I checked the typical installation location here (C:\Users\user\AppData\Local\Apps\Janet\Library/jpm and C:\Users\user\AppData\Local\Apps\Janet\bin) to veritfy there the jpm-related files were not there.

I also verified after uninstalling jpm that invoking jpm resulted in the mesage 'jpm' is not recognized as an internal or external command, operable program or bathc file.

I also checked after pulling the latest jpm and installing it that shutil.janet had the changes in 588485be0c834d39ade9a967f254f9ca3a373082

There may still be pilot error here but if there is I haven't figured out what it is.

sogaiu commented 3 years ago

@bakpakin When I evaluate (os/environ) via jpm debug-repl, the resulting table has a key Path, but not a key for PATH. Perhaps that is to be expected.

When the merge-into in shell completes, the resulting table has both an entry with key Path and an entry with key PATH -- and the corresponding values differ.

I don't know what's going on with the subsequent os/execute, but I get the error output mentioned originally.

As a test, I changed the PATH key to Path for the struct being merged in. This results in error-less output.

I have tried examining the keys in the table that os/environ returns in different janet repl invocations and it appears that the key that ends up there can vary (i.e. sometimes it is Path and sometimes it is PATH).

sogaiu commented 3 years ago

Curiously, merging in a struct with both Path and PATH didn't seem to work here (same error message).

Checking the return value of os/environ to see if Path or PATH is already present and using whichever key was there (or if neither was there, just using PATH), seems to work:

  (def os-env (os/environ))
  (def path-key
    (if (get os-env "Path")
      "Path"
      # "PATH" keys exists or neither "Path" nor "PATH" keys exist
      "PATH"))
  (def env (merge-into os-env {"JANET_PATH" (dyn:modpath)
                               path-key (patch-path (os/getenv "PATH"))}))

(I replaced: https://github.com/janet-lang/jpm/blob/996a59e28ff758de5b73822dafa8e343e9879958/jpm/shutil.janet#L100-L101 with the eariler code in my successful test.)

sogaiu commented 3 years ago

Thanks for the change in 1cd77f978afab5c722e61c6f3cf8adb1dfbafa90 -- that worked here.