Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Improved readme, installation instructions, and documentation. #20

Merged
merged 1 commit into from Sep 16, 2013

Conversation

Projects
None yet
2 participants
Contributor

sun commented Sep 8, 2013

As promised, here we go :-)

Use the "View file @ ..." link provided by the GitHub PR user interface to see the final result.

Proposed procedure:

  1. Let me know what you think and want to change or what you don't feel comfortable with.
  2. I'll add commits to this PR.
  3. Once we're happy, I'll rebase + squash commits into a single to remove useless history.
Contributor

sun commented Sep 8, 2013

As mentioned in #19, I also clarified the difference to Charade here. In fact, an explanation for the keychain approach was contained already — only the relation to Charade was missing ;-)

@cuviper cuviper and 1 other commented on an outdated diff Sep 12, 2013

@@ -7,7 +7,7 @@ PROGRAM = ssh-pageant.exe
SRCS = main.c winpgntc.c
HDRS = winpgntc.h
MANPAGE = ssh-pageant.1
-DOCS = README COPYING COPYING.PuTTY
+DOCS = README.md LICENSE LICENSE.PuTTY
@cuviper

cuviper Sep 12, 2013

Owner

The COPYING->LICENSE change is not needed. Either filename could be ok, but most GPL projects use COPYING as the FSF recommends. There's no reason to change what's already here.

@sun

sun Sep 12, 2013

Contributor

Yeah, I already figured that this would probably be too much when I ultimately committed... ;)

Cause: GitHub recommends to place a LICENSE file into every repo and many other FOSS projects use that name (long before GitHub) already.

Sorry, I clearly was too overzealous :) Will revert!

@cuviper cuviper and 1 other commented on an outdated diff Sep 12, 2013

+
+
+## Installation
+
+The easiest way to install `ssh-pageant` is through the readily-available [binary
+releases](https://github.com/cuviper/ssh-pageant/releases):
+
+* Choose the `vX.Y-prebuilt64` build if
+ * you are running a 64bit version of Windows
+ * **and** you installed [Cygwin] for the 64bit architecture (typically in
+ `c:\cygwin64`).
+* Choose the `vX.Y-prebuilt` build if
+ * you are running a 32bit version of Windows
+ * **or** you installed [Cygwin] for the 32bit architecture (typically in
+ `c:\cygwin`)
+ * **or** in case you don't know. :)
@cuviper

cuviper Sep 12, 2013

Owner

I think this explanation is needlessly complex. Just provide direct links to the v1.3 packages, and we can update the version when there's a new release. And I'd take the lead from cygwin.com on simply providing 32-bit and 64-bit links without getting bogged down in the explanation. Maybe just hint to go 32-bit if you're not sure.

@sun

sun Sep 12, 2013

Contributor

I can see that approach, too.

The unique trigger for adding that info was that I'm on 64bit myself, but I actually spent quite some time with trying to figure out what build of Cygwin I'm actually running. ;) Worse, I did not actually find an answer for how to retrieve that info in an existing install in the Cygwin docs. In the end, I noticed the difference in the (default) installation directory name. The idea was that this hint might help other users to figure this out more quickly, too. :)

However, definitely not married to this elaborate description. I agree we can put the current version numbers (+ links) directly in there, since those will change only rarely.

I'll come up with a revised proposal.

@cuviper

cuviper Sep 12, 2013

Owner

Try uname -m, if you're really not sure. :)

@sun

sun Sep 12, 2013

Contributor

Yeah, I had also tried that already, but doesn't really give a clue:

$ uname -a
CYGWIN_NT-6.1-WOW64 sw10 1.7.24(0.269/5/3) 2013-08-15 11:55 i686 Cygwin

AFAIK, the 64bit build of Cygwin was only introduced "recently". When I originally installed Cygwin, I'm fairly sure that the differentiation did not exist yet. uname appears to display the hardware aspects (64bit) only.

@cuviper

cuviper Sep 13, 2013

Owner

Were you inferring "hardware aspects" from the WOW64 bit? That is Microsoft's misleading name for the 32-bit subsystem running on Win64. If you run "native" Cygwin, either 32-on-32 or 64-on-64, it just says "CYGWIN_NT-6.1". But the meat of this uname -a line is near the end, "i686", which is what uname -m would isolate for you. 64-bit Cygwin will instead report "x86_64" for the machine name.

@cuviper cuviper commented on an outdated diff Sep 12, 2013

+## Usage
+
+1. Ensure that PuTTY's Pageant is running (and holds your SSH keys).
+ * ssh-pageant does not start Pageant itself.
+ * Recommended: Add Pageant to your Windows startup/Autostart configuration
+ so it is always available.
+
+1. Edit your `~/.bashrc` (or `~/.bash_profile`) to add the following:
+
+ # ssh-pageant
+ eval $(/usr/bin/ssh-pageant -ra /tmp/.ssh-pageant)
+
+ To explain:
+ * This leverages the `-r`/`--reuse` option (available since 1.3) in
+ combination with `-a SOCKET`, which will only start a new daemon if the
+ specificied path does not accept connections already. If the socket
@cuviper

cuviper Sep 12, 2013

Owner

*specified

@cuviper cuviper commented on an outdated diff Sep 12, 2013

+ To explain:
+ * This leverages the `-r`/`--reuse` option (available since 1.3) in
+ combination with `-a SOCKET`, which will only start a new daemon if the
+ specificied path does not accept connections already. If the socket
+ appears to be active, it will just set `SSH_AUTH_SOCK` and exit.
+ * When using this, the `ssh-pageant` daemon remains to be active (and
+ visible in your task manager). You should not kill the process, since
+ open shells might still be using the socket.
+ * Usage of `eval` to load the environment variables is the usual approach.
+ By default, ssh-pageant outputs sh-style commands. Use the `-c` option
+ for csh-style commands.
+ * To share the socket between Cygwin and [msysgit/MinGW](http://msysgit.github.io/),
+ ensure to use a path that resolves to the same location in both
+ environments via `cygpath --windows {path}`
+
+3. Profit.
@cuviper

cuviper Sep 12, 2013

Owner

At least set up the joke correctly:

  1. (step one)
  2. (step two)
  3. ????
  4. Profit!!!

But while I appreciate the humor, it's out of place with the rest of the document.

@cuviper cuviper closed this Sep 12, 2013

@cuviper cuviper reopened this Sep 12, 2013

Contributor

sun commented Sep 12, 2013

All previous review issues should be fixed now. Let me know if you see any further.

Regarding the github.io pages, I'd rather recommend to get rid of them. When I searched for a solution myself, neither this repo nor the github.io pages appeared in the results. My entry path was through various blog and forum posts on the net, which all linked to the github repository (not the github.io page). So in terms of backlinks, the repository page certainly must have a much higher rank anyway already.

Thanks!

Contributor

sun commented Sep 16, 2013

Squashed + ready for merge :)

cuviper added a commit that referenced this pull request Sep 16, 2013

Merge pull request #20 from sun/readme
Improved readme, installation instructions, and documentation.

@cuviper cuviper merged commit 5768f91 into cuviper:master Sep 16, 2013

Owner

cuviper commented Sep 16, 2013

Thanks for your contribution!

@sun sun deleted the sun:readme branch Sep 16, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment