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

Native EsyBash: 'ls' command doesn't work #24

Closed
bryphe opened this issue Oct 20, 2018 · 6 comments
Closed

Native EsyBash: 'ls' command doesn't work #24

bryphe opened this issue Oct 20, 2018 · 6 comments

Comments

@bryphe
Copy link
Collaborator

bryphe commented Oct 20, 2018

With the native version of esy-bash, the ls command doesn't work:

E:\esy-bash\re\_build\default\bin [master ≡ +1 ~0 -0 !]> ./EsyBash.exe ls -a
C:\Users\bryph\AppData\Local\Temp\__esy-bash__79677993__1__.sh: line 2: ls: command not found

However, this works correctly with the node version:

E:\esy-bash [master ≡ +1 ~0 -0 !]> node .\bin\esy-bash.js ls -a
.        .git         __tests__     bin              jest.config.js  package.json       re
..       .gitignore   appveyor.yml  index.js         LICENSE         package-lock.json  README.md
.cygwin  .travis.yml  bash-exec.js  install-opam.sh  node_modules    postinstall.js

It looks like the difference is that the JS version is somehow including /usr/local/bin and /usr/bin` in the PATH, whereas the native version is not. I'm not sure if this is some difference in how we spin up the bash process? We'll need to make sure these are included in the PATH

@ManasJayanth
Copy link
Member

ManasJayanth commented Oct 21, 2018

Weird. Works fine here.

c:\Users\manas\development\esy-bash\re>.\_build\default\bin\EsyBash.exe ls
_build  appveyor.yml  dune-project      esy-bash.opam  lib           package.json
_esy    bin           esy-bash.install  esy.lock.json  node_modules  tests

Infact thats how I used to make sure it was working. Nevertheless, looking into it.

@ManasJayanth
Copy link
Member

Turnout it was because of cygwin on my path that didn't let me know of this issue.

@bryphe
Copy link
Collaborator Author

bryphe commented Oct 22, 2018

Turnout it was because of cygwin on my path that didn't let me know of this issue.

Oh interesting, so it picked up the other cygwin install - thanks for the help with this @prometheansacrifice !

ManasJayanth added a commit that referenced this issue Oct 22, 2018
ManasJayanth added a commit that referenced this issue Oct 22, 2018
Also fixes missing symlink config for cygwin
ManasJayanth added a commit that referenced this issue Oct 22, 2018
Also fixes missing symlink config for cygwin
@ManasJayanth
Copy link
Member

Writing tests for this will be a little tricky since the cygwin binaries are already on the path when run in the test environment.

@bryphe
Copy link
Collaborator Author

bryphe commented Oct 22, 2018

Thanks for the fix @prometheansacrifice !

Writing tests for this will be a little tricky since the cygwin binaries are already on the path when run in the test environment.

This is because we run the tests via esy b dune runtest? So the paths already get included by esy?

One alternative would be to continue using node/javascript for our tests - we could run those outside of the esy sandbox. We'd just have to change the way that we invoke esy-bash - using our executable instead of the JS script.

Another option would be that we could run the built Reason test_runner executable outside of the esy sandbox.

Or perhaps it makes sense to have two classes of tests - inline unit tests (Reason), end-to-end tests (Node runner)?

@bryphe
Copy link
Collaborator Author

bryphe commented Nov 1, 2018

Thanks for the fix, @prometheansacrifice ! Verified it works now (and we have integrated esy-bash native with master on esy! 🎉 )

@bryphe bryphe closed this as completed Nov 1, 2018
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

No branches or pull requests

2 participants