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

Install clojure cli tools to a location common to powershell & pwsh #62

Merged
merged 5 commits into from
Jul 21, 2022

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Jul 21, 2022

Hi,

could you please consider patch to install the Clojure cli tools to a common PowerShell modules location that is discoverable by both the powershell and pwsh shells. It addresses #61.

This is done by setting the PSModulePath variable to the common location before installing the Clojure cli tools on MS-Windows. The Clojure cli tools install script looks into that variable to present the user with the list of paths to install to, and this action chooses option 1, i.e. this path.

I've generated the js script code by running npm run all, and I've also updated the examples in smoke tests and the readme file, not sure if there are more references.

The smoke tests should pass once committed to master, not sure how to run them from the branch.

Thanks

@DeLaGuardo
Copy link
Owner

Looks like it is still failing smoke test
Could it be because exec is running a new powershell here? I'm not a windows user so it is hard for me to reason why it is happening

@ikappaki
Copy link
Contributor Author

Looks like it is still failing smoke test
Could it be because exec is running a new powershell here? I'm not a windows user so it is hard for me to reason why it is happening

It is failing because the smoke test in my PR branch references the master branch which doesn't have the fix. Would you like to change the smoke testa action to reference the PR branch so that it passes, and before we are about to merge I switch the reference back to master?

Thanks

@DeLaGuardo
Copy link
Owner

Looks like it is still failing smoke test
Could it be because exec is running a new powershell here? I'm not a windows user so it is hard for me to reason why it is happening

It is failing because the smoke test in my PR branch references the master branch which doesn't have the fix. Would you like to change the smoke testa action to reference the PR branch so that it passes, and before we are about to merge I switch the reference back to master?

Thanks

right, sorry, I should check that first :)
yes, please check with your repo/branch first

@DeLaGuardo
Copy link
Owner

Amazing! I see it works now.

@ikappaki
Copy link
Contributor Author

right, sorry, I should check that first :) yes, please check with your repo/branch first

Done, could you please have another look to confirm?

@DeLaGuardo DeLaGuardo self-requested a review July 21, 2022 11:00
Copy link
Owner

@DeLaGuardo DeLaGuardo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It also allows proper caching for ClojureCLI on windows runners :)

@DeLaGuardo DeLaGuardo merged commit 3410b07 into DeLaGuardo:main Jul 21, 2022
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