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

make it work on windows #95

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

markus-wa
Copy link

@markus-wa markus-wa commented Mar 22, 2020

This PR makes lein-tools-deps work on windows.

see also #85

NOTE: clojure.bat needs to exists on the PATH: NO LONGER REQUIRED WITH LATEST CHANGES

@ECHO OFF
call powershell.exe -c "clj %*"

TODO: done

  • add note in README regarding windows
  • fix failing tests on windows

Copy link
Owner

@RickMoynihan RickMoynihan 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; I'll definitely consider merging it with a bit more information about what it is doing; and some appropriate instructions in the README for Windows users.

It also seems that the clj tools on windows themselves are in the early days...

What assumptions does this make about your setup? For example should clj be installed like this?

These instructions don't seem to assume a clojure.bat is it possible to make this PR work without it?

src/lein_tools_deps/env.clj Outdated Show resolved Hide resolved
src/lein_tools_deps/env.clj Outdated Show resolved Hide resolved
@markus-wa
Copy link
Author

markus-wa commented Mar 25, 2020

Maybe we could do something like cmd /c "powershell.exe -m clojure ... instead of requiring clojure.bat will give it a go when I'm back on a windows machine at some point.

- use 'powershell -c' to invoke Clojure CLI Tools when on windows
- escape backslashes found in windows paths
- make tests run on windows
- add section to README abount installing the CLI tools on windows
@markus-wa
Copy link
Author

@RickMoynihan I've made some changes according to your suggestions.

  • use 'powershell -c' to invoke Clojure CLI Tools when on windows (this means we no longer need clojure.bat!)
  • comment about backslashes
  • made tests run on windows
  • added section to README

@markus-wa markus-wa changed the title WIP: make it work on windows make it work on windows Mar 28, 2020
@markus-wa
Copy link
Author

@RickMoynihan let me know if there's anything else I can do for this PR 🙂

@RickMoynihan
Copy link
Owner

@markus-wa thanks sorry I'd missed the notification on this repo. Will try to find some time to take a look at it and think it through.

At a quick glance the changes look reasonable, but my main problem is I don't use windows, or have the time to test it there myself.

Having some automated tests on windows would be useful to prevent any regressions, and help ensure this was maintained. I'd rather not mix that with this PR though, that could be added after this PR if someone would be willing to contribute such a thing.

@RickMoynihan
Copy link
Owner

Thanks for the updates... will try and find some time to think it through and get this merged.

@markus-wa
Copy link
Author

Awesome, cheers @RickMoynihan

Btw, I played around with windows builds on travis yesterday but it seems like it's not yet working for clojure. The build just errored before even booting the runner.

@terohe
Copy link

terohe commented Nov 10, 2020

Any update on this? I tested this locally on Windows and seems to work like a charm.

:middleware [lein-tools-deps.plugin/resolve-dependencies-with-deps-edn]

@markus-wa
Copy link
Author

no idea @terohe - I switched to Linux in the meantime tbf, but would still be cool to see this merged @RickMoynihan

@RickMoynihan
Copy link
Owner

Hi,

I don't actually use lein-tools-deps anymore and I'm conscious that tools.deps itself has changed quite a lot recently to support different aliases. So I don't feel like I'm the best maintainer for this project anymore.

If the project is still useful to people I'd be happy to see someone fork and maintain the project.

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.

3 participants