-
Notifications
You must be signed in to change notification settings - Fork 6
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
Simplest support of Windows with Docker #5
Conversation
The combination of docker and MSYS2 doesn't seem to convert Windows path properly Converting the path explicitly with cygpath worked Tried with Docker version 17.04.0-ce, build 4845c56
Thanks for your contribution! Could you extend the README to show how to use it? |
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.
Thanks! I've commented a few places.
README.md
Outdated
$ cd viewprof/docker | ||
$ docker-machine start | ||
$ docker build -t viewprof . | ||
$ .\viewprof.bat path/to/file.prof |
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.
Does Windows accept forward slash delimiters?
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.
Yes, actually whichever will do.
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.
That's good to know.
README.md
Outdated
|
||
#### How to install and launch | ||
|
||
```bash |
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.
Should we use bat
or something instead? Or is it actually bash?
README.md
Outdated
#### How to install and launch | ||
|
||
```bash | ||
$ git clone git@github.com:maoe/viewprof.git |
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.
Please drop the $-signs to make the instructions copy-paste friendly.
README.md
Outdated
@@ -17,12 +17,31 @@ It has three display modes: | |||
|
|||
## Installation | |||
|
|||
Note: Currently viewprof doesn't support Windows because the underlying library (vty) doesn't support it yet. See [#1](https://github.com/maoe/viewprof/issues/1). | |||
For Windows users, see below. |
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.
Let's drop this sentence. The following instruction is short.
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.
LGTM, thanks!
Squashed last three commits and merged into feature/docker. |
May fix #1
But there're still problems:
cygpath
(tool to convert Windows path into the POSIX one)Tried with: Docker Toolbox v17.04.0ce