-
Notifications
You must be signed in to change notification settings - Fork 111
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 pwsh support #537
Add pwsh support #537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for this contribution. That's great work.
In addition to the code review comment, main point is to add support in the installation testsuite (make testinstall). You will find it in testsuite/install.00-init/
. This testsuite will check that pwsh
correctly defines variable, function, etc with the commands generated by modulecmd.tcl
On the documentation side, things to add
- How to initialize module on pwsh
pwsh
value returned bymodule-info shell
pwsh
value returned bymodule-info shelltype
- update Shell support section
Support for source-sh
or sh-to-mod
commands may also be added (translate a pwsh script into a modulefile). But it may be the topic of another pull request.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #537 +/- ##
=======================================
Coverage 99.50% 99.50%
=======================================
Files 15 15
Lines 5453 5474 +21
=======================================
+ Hits 5426 5447 +21
Misses 27 27 ☔ View full report in Codecov by Sentry. |
@xdelaruelle I've added pwsh to the othlang_list in the installation tests because it often doesn't behave like a normal shell. Also, it was easier to write the |
@xdelaruelle I think I addressed everything now except the support for |
41d531b
to
aed55dc
Compare
Many thanks Simon for this deep work. I will take over if I spot some finishing changes to do. I will test everything on my side. |
The pwsh initialization works similar to the cmd initialization. Autoinit will not work on Windows due to the hard-coded paths in modulecmd.tcl. On other platforms the autoinit should work for pwsh.
aed55dc
to
43efc18
Compare
43efc18
to
f37e233
Compare
@ACSimon33 module quarantine tests are failing so I suspect there is an issue with the code of this feature. Could you please look at it to fix it. If this is too hard or time-consuming for you, I may remove the feature for the moment (could be added later on). Beware that I have reworked the commits of your branch, so you may pull these changes first, prior to push new commits. |
@xdelaruelle Is it ok if we remove the environment variable from the INSTALL.bat script? Alternatively we could create a new install script for pwsh which just copies the files and doesn’t set the env variable. That way nothing would change for CMD users. |
- LastExitCode is now always set to 0 or 1, depending on the success of the command. - If there is an error we throw an exception in envmodule and ml. - If the subcommand is a query, e.g. `is-avail`, we return a boolean value.
Add support for the quarantine mechanism with pwsh shell. Init script init/pwsh.ps1 does not handle quarantine on Windows platform.
There is no pwsh support there and if the installation test suite find the windows pwsh.exe it will fail due to IO issues.
cd85979
to
d590de8
Compare
@ACSimon33 I prefer no change for CMD users. Could you propose specific INSTALL/TESTINSTALL/UNINSTALL scripts for pwsh? Subsidiary question: is there a way to automatically setup |
Alright, I will create the corresponding pwsh scripts.
The path to the profile script for pwsh is stored in the |
8043ad5
to
c17210a
Compare
Many thanks again @ACSimon33 for your deep work on this. Everything seems ready. I will merge the PR as soon as the CI completes. |
Hi,
Thanks |
@TaiXeflar |
Hi,
since there seemed to be no progress on the pwsh support ticket, I thought I'd just try it myself.
I've took the hints from the issue and addressed everything which is applicable to pwsh:
Aliases are defined as functions as well because pwsh doesn't support aliases that contain whitespaces or consist of multiple commands that take arguments.
The two above are not applicable to pwsh as far as I can tell (at least on Windows)
The autoinit works only on Unix platforms. On Windows, I defined the the environment and the
envmodule
command directly in the init script.I've tested everything manually on Windows and Linux but I still need to get the testsuite running for pwsh. Also, I probably need to add something to the documentation. In the meantime, any feedback is appreciated.
Best,
Simon
Closes #326