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

Change Windows build documentation #4691

Merged
merged 1 commit into from
Apr 24, 2018
Merged

Conversation

djdv
Copy link
Contributor

@djdv djdv commented Feb 13, 2018

I rewrote this. Maybe it's better this way?
I'm not sure if the table alignment looks fine or crazy. Other options for that: left aligned table, separated command blocks with text annotations above, single paragraph explaining what will happen followed by a single command block.

I also have graphics that could be added but they're probably unnecessary.
Embed underneath "Building on Windows": https://ipfs.io/ipfs/QmccXW7JSZMVXidSc7tHsU6aktuaiV923q4yBGHUsdymYo/build.gif
Add as a reference/link near the Cygwin setup portion: https://ipfs.io/ipfs/QmaYFSQa4iHDafcebiKjm1WwuKhosoXr45HPpfaeMbCRpb/cygwin%20-%20install.png

I added MSYS2 as a method, and basically redid everything else. I feel like it is more concise than it was previously, even with the repetition/explanations.
Criticisms welcomed.

Blocked on: #4682

@djdv
Copy link
Contributor Author

djdv commented Feb 13, 2018

I removed the "eval loops" FOR /F "tokens=*" %E IN ('go env') DO %E, from the make method, their purpose was just to make sure the environment variables for go (specifically GOPATH) were set but it's probably better to just demand that the user set it themselves.

Unfortunately there's no better way to take stdout and place it into an environment variable (that I know of) so it has to remain to store the git hash before building in the "minimal" section.

This commit will be squashed later.

docs/windows.md Outdated
You can check that the ipfs you built has the right version using:
The syntax for the next command is different depending on how it's executed. If you're on the interactive command line vs executing from inside the batch/cmd interpriter.
interactive: `FOR /F %V IN ('git rev-parse --short HEAD') do set SHA=%V`
interpriter: `FOR /F %%V IN ('git rev-parse --short HEAD') do set SHA=%%V`
Copy link
Member

Choose a reason for hiding this comment

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

typo: interpreter

Copy link
Contributor

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

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

Hi @djdv, @whyrusleeping asked if I could take a quick look at this (I’m currently working on rethinking how docs are managed across IPFS), so I added some thoughts below.

I also wanted to add a quick note that it would be great to know what other folks here think about the way you are using tables to put descriptions besides each line of code. It’s a really neat idea, but I worry that the repetition is confusing. I also think the way GitHub renders it makes it a bit noisy and hard to read.

docs/windows.md Outdated
## Make
There are currently 2 documented ways of building with `make` on Windows.
[MSYS2→](#msys2)
[Cygwin→](#cygwin)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this section doesn’t really contain additional info and is just branching, I think it might make sense to get rid of it entirely and move these into the “choose the way you want to proceed” section. So you’d have 3 links there:

  • Make with MSYS2 →
  • Make with Cygwin →
  • Minimal →

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, is there any [short] advice you can add here or link to about why someone might want to choose MSYS2 or Cygwin over the other? For someone new to both, this is could be a confusing decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure about adding advice here, it feels subjective. While there are technical differences, at the end of the day both provide the posix tools we need to build go-ipfs. I think the major decision comes down to what users already have installed, if it's neither, the differences between them are just going to be the differences between the 2 sections, which is not much. Mostly it just depends on if you like a graphical package manager or a command line one (*for our purposes).

I'll think this over and see if I can come up with something appropriate. Maybe defer to an external resource comparing the 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the major decision comes down to what users already have installed

Absolutely. But that doesn’t cover a large group of people.

I'm unsure about adding advice here, it feels subjective.

It’s totally subjective :)

The problem is that you are starting the whole tutorial with a choice, and, in particular, one that has nothing to do with the task at hand (building IPFS) and one users may likely not know enough about in order to even know how to choose. That can stop people from following the tutorial right at the beginning. (And even for someone enterprising, it adds a lot of extra work: now they have to go research each of these options in order to figure out which to choose.)

It’s OK for it to be subjective (and you can even say so). The goal is to remove the barrier of this choice. This could be as simple as “all these options are good; if you don’t already have one of these tools installed or aren’t familiar with them, go with MSYS2 because it makes the process easy” or “go with ‘minimal’ because you don’t have to install anything extra.”

docs/windows.md Outdated

You can permenantly append the MSYS2 bin to your `PATH` so that on subsequent builds you can simply run
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an actual command you can include here to do that? Or an article/tutorial on it you can link to?

By itself, this sentence leaves a novice hanging, not knowing how to enact the advice, and probably doesn’t tell an expert anything new.

(From my recollection of doing Windows dev years ago, this was non-trivial and buried in system settings somewhere.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my recollection of doing Windows dev years ago, this was non-trivial and buried in system settings somewhere

The biggest problem is that it's different across versions, even on Windows 10 alone I've noticed they changed the interface 3 times now.

There's the setx command but I need to test access and availability of it. I think users can invoke it normally and I'm seeing Windows 7 as the minimum version but it may only be included with a specific SDK until Windows 8. If it's included in stock Windows 7 I'd be comfortable just recommending that as the means. The only problem I can think of is that it would be bad if users made a typo for their PATH
i.e
setx PATH %PATH;\some\other\path
notice the lack of the closing % which would just ruin everything. I guess that can't really be avoided either way though.

I'll write a revision including this and maybe a notice "be very careful to check your input here..."

Copy link
Contributor

Choose a reason for hiding this comment

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

The biggest problem is that it's different across versions, even on Windows 10 alone I've noticed they changed the interface 3 times now.

Is this a good resource to link to? https://www.computerhope.com/issues/ch000549.htm

docs/windows.md Outdated
steps below.
## Cygwin
Install Cygwin (https://www.cygwin.com)
During the install, select the devel packages `git` & `make`, archive package `unzip`, and net package `curl`
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a little clearer to scan and reference as a bulleted list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, how should someone install these packages if they already have Cygwin? (If I recall, the answer is run the Cygwin setup again, but not sure if things have since improved.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right about needing to (download and) run the setup binary again, even if you already have it installed. I can see about adding some kind of wording like "Even if you already have Cygwin installed, you'll need to run the setup file again to add these packages"

docs/windows.md Outdated

## Manual Way: download and install dependencies
You can permanently append the Cygwin bin to your `PATH` so that on subsequent builds you can simply run
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note as in the MSYS2 section: is there an actual command you can include to demonstrate this or an article/tutorial on it you can link to?

docs/windows.md Outdated
go get -u github.com/whyrusleeping/gx
go get -u github.com/whyrusleeping/gx-go
go get -u -d github.com/ipfs/go-ipfs
cd %GOPATH%/src/github.com/ipfs/go-ipfs
gx --verbose install --global
Copy link
Contributor

@Mr0grog Mr0grog Feb 14, 2018

Choose a reason for hiding this comment

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

I think it might be clearer if you separate out:

  1. Installing gx/gx-go
  2. Using gx to install the dependencies for go-ipfs

You can also then just say “skip this section” if using prebuilt binaries instead of telling the reader which commands in the middle of the example to ignore :)

docs/windows.md Outdated
"%GOPATH%\bin".

You can check that the ipfs you built has the right version using:
The syntax for the next command is different depending on how it's executed. If you're on the interactive command line vs executing from inside the batch/cmd interpriter.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be helpful to preface this with a statement of what you’re doing. For example: “next, you need to set the SHA environment variable, which is used when building.”

docs/windows.md Outdated
```

It should output something like "ipfs version 0.4.0-dev-XXXXXXX" where
XXXXXXX is the current commit that you passed to the build command.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should keep the step of verifying the install here. (You could remove the “where
-XXXXXXX is the current commit that you passed to the build command” bit since you’ve abstracted that away with a command + env var.)

It might also be useful to give some basic advice on what to do if they don’t get the expected output, even if it’s just “post to https://discuss.ipfs.io .”

docs/windows.md Outdated

```
ipfs version --commit
go install -ldflags="-X "github.com/ipfs/go-ipfs/repo/config".CurrentCommit=%SHA%"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would definitely be helpful to note that this is the actual build step. It could be as simple as just saying “finally, actually build and install go-ipfs.”

docs/windows.md Outdated
`go-ipfs` is built on Golang and thus depends on it for all building methods.
https://golang.org/doc/install
The `GOPATH` environment variable must be set as well.
https://golang.org/doc/code.html#GOPATH
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also still need to set GOROOT? I think the previous version of this section was a little bit more explanatory for someone who might not be familiar with building Go programs. I know when I first approached Go, I found the various environment variables it required surprising (the equivalents are typically optional customizations with many other languages).

It might make sense to keep most of the previous version of this section as-is or tweak it slightly. Keeping it short is good, but this version strikes me as a little too terse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Go installer on Windows sets GOROOT for us. I don't remember the exact release that started doing this but it's certainly before our minimum required version for building. I'm leaning towards keeping mention of GOROOT out since it should be set already and there's a reference to the Go documentation which is updated independently to include any necessary steps to setup the environment as they change with the language.

Maybe adding a comment to reinforce/advising users read it like: "Make sure to read the documentation at https://golang.org/doc/install and https://golang.org/doc/code.html#GOPATH to ensure your environment is properly setup for building".

The above command uses Git to download go-ipfs from its GitHub
repository. If you get authentication problems with Git, you might
want to take a look at
https://help.github.com/articles/caching-your-github-password-in-git/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still a common issue? If so, it seems like the advice here should be kept, maybe in a trouble-shooting section at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure the origin of this. Even years ago I never encountered this on any of the environments I used for testing. It may have been an issue upstream that was resolved.

Either way the troubleshooting section is a good idea for all misc. comments.

@djdv
Copy link
Contributor Author

djdv commented Feb 16, 2018

Thinking out loud.
I'd really love some method of annotation for the code blocks, tables were just the best I could come up here. In an ideal state I'd love them to be hidden/revealed somehow, like if I could have a toggle button that displays the annotations but still maintain a code block people can select and copy from when the annotations are collapsed/hidden.

@djdv
Copy link
Contributor Author

djdv commented Feb 16, 2018

Draft updated. I plan on looking into making a comment for Msys2 vs Cygwin, I may just insert an "it's preferential" type statement.
I need to do a consistency/grammar and visual/style review myself but making remark before I get to it would be welcomed.
The graphical header was added as well as a reference image link for Cygwin. I'd like a 👍 or 👎 on the graphical header, not sure if it's too much.
If I missed anything else, please remark on it.

@djdv
Copy link
Contributor Author

djdv commented Feb 26, 2018

@Mr0grog I think I've addressed all that was mentioned, I'd like another pass at this when you get a chance.

After all is cleared I'll squash and sign-off the commits.

@djdv djdv force-pushed the docs/windows branch 2 times, most recently from ee0b085 to d6501a4 Compare February 26, 2018 16:49
@djdv
Copy link
Contributor Author

djdv commented Feb 26, 2018

Sorry for the whitespace noise, my editor seems to have inconsistencies with GitHub's renderer.

docs/windows.md Outdated

The following commands should download or update go-ipfs dependencies
and then install them:
You can check that the ipfs output versions match with `go version` and `git rev-parse --short HEAD`, if `ipfs.exe` executes and everything matches, then building was successful.
Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn’t quite clear to me that “the ipfs output versions” meant the output of the last line above until I read this a second time. Maybe you could reword this to make that a little clearer, like:

The last command should output something like: go-ipfs version: 0.4.14-dev-XXXXXXX. The XXXXXXX part should match the output you get if you run git rev-parse --short HEAD. (That is, the ipfs command you built should have the same version number as the source code.)

Alternatively, is this bit really needed? Is there a likely scenario when someone won't see an error message during the build steps but still winds up with their built binary referencing something different than the version of the source they’ve got?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with needing clarity in the first half, as for if this section is necessary, I can lean either way, it serves to test and verify the binary is what it's expected to be. It's similar to part of the original document

You can check that the ipfs you built has the right version using...
It should output something like "ipfs version 0.4.0-dev-XXXXXXX" where XXXXXXX is the current commit that you passed to the build command.

I'll combine this with

The XXXXXXX part should match the output you get if you run git rev-parse --short HEAD

docs/windows.md Outdated

## Download go-ipfs and fix Git authentication
Using `make`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Bolding this header might help make this section of the doc easier to scan. (Same for “using build tools manually.”)

docs/windows.md Outdated

## Download go-ipfs and fix Git authentication
Using `make`:
These sections cover setting up the build environment, each option is equally valid and differ only in their setup process. If you have no preference or special requirements, MSYS2 is recommended as the first choice.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully it’s already clear to people that this is about setting up their build environment :) The thing that might be more useful to clarify here is what MSYS2 and Cygwin are, e.g:

MSYS2 and Cygwin make it possible to run Unix-style programs (like make) on Windows. Both are great, but if you aren’t already using one, we recommend MSYS2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes I'm a little redundant and robotic...
I like the more human friendly wording here but I have qualms with saying Cygwin is "great" ;^)
How does this revision sound?

MSYS2 and Cygwin provide the Unix tools we need to build go-ipfs. You may use either, but if you don't already have one installed, MSYS2 is recommended.

P.S.
I'm a little robotic and redundant sometimes.

docs/windows.md Outdated
## Manual Way: build go-ipfs

To actually build go-ipfs, first go to the cmd/ipfs directory:
You can check that the ipfs output versions match with `go version` and `git rev-parse --short HEAD`, if `ipfs.exe` executes and everything matches, then building was successful.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above from the MSYS2 section.

docs/windows.md Outdated

### `gx`
`gx` and its go-hook `gx-go` can be built by us.
Alternativley you may use prebuilt binaries ([gx](https://dist.ipfs.io/#gx), [gx-go](https://dist.ipfs.io/#gx-go)) by placing them in your `PATH` and skipping this section.
Copy link
Contributor

Choose a reason for hiding this comment

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

Flipping this sentence with the previous one places the text about building from source next to the instructions for doing so. That can help clarify exactly what the code block below is supposed to be. e.g:

You can install prebuilt binaries for gx and gx-go and skip this section. Otherwise, you can install and build them from source by running the following:

@djdv
Copy link
Contributor Author

djdv commented Feb 27, 2018

Addressed review comments, moved the "test binary" sections up, added table headers.

@djdv djdv mentioned this pull request Mar 13, 2018
9 tasks
@djdv
Copy link
Contributor Author

djdv commented Mar 27, 2018

@Mr0grog, @nothingismagick
Can you take a look at this?

docs/windows.md Outdated
you have created, and the %GOPATH/bin directory should also be in the
Path environment variable.
## Choose the way you want to proceed
`go-ipfs` utilizes `make` to automate builds and testing, but can be built without it using only `git` and `go`.
Copy link

@nothingismagick nothingismagick Mar 27, 2018

Choose a reason for hiding this comment

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

"to automate builds and run tests,"

docs/windows.md Outdated
Path environment variable.
## Choose the way you want to proceed
`go-ipfs` utilizes `make` to automate builds and testing, but can be built without it using only `git` and `go`.
No matter which section you choose, if you encounter issues, please see the [Troubleshooting](#troubleshooting) section.

Choose a reason for hiding this comment

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

"No matter which method you choose, ..."

The troubleshooting section only mentions git auth stuff and otherwise sends people to discuss.ipfs.io - it might be helpful to also list a few possible gotchas (sry - with windows I can't help there)

docs/windows.md Outdated

You can check that the ipfs you built has the right version using:
- Anything else
Please search <https://discuss.ipfs.io/> for any additional issues you encounter, if you can't find an existing resolution, feel free to make a post asking for help.

Choose a reason for hiding this comment

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

"Please search https://discuss.ipfs.io/search?q=windows%20category%3A13 for any additional issues you may encounter. If you can't find any existing resolution, feel free to post a question asking for help."

Of course, if they are behind the great firewall, this service will not be available - but then again neither will github...

docs/windows.md Outdated
```
ipfs version --commit
```
If you encounter a bug with `ipfs` itself (not related to building) please use the issue tracker to report it.
Copy link

@nothingismagick nothingismagick Mar 27, 2018

Choose a reason for hiding this comment

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

If you encounter a bug with go-ipfs itself (not related to building) please use the issue tracker to report it.

docs/windows.md Outdated
```

## Manual Way: build go-ipfs
If there were no errors, the final command should output version information simillar to "`ipfs version 0.4.14-dev-XXXXXXX`" where "XXXXXXX" should match the current short-hash of the `go-ipfs` repo. You can retrieve said hash via this command: `git rev-parse --short HEAD`. If `ipfs` runs and the version string matches, building was successful.
Copy link

@nothingismagick nothingismagick Mar 27, 2018

Choose a reason for hiding this comment

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

"information similar to"

docs/windows.md Outdated

## Cygwin
1. Install Cygwin (https://www.cygwin.com)
2. During the install, select the following packages. (If you already have Cygwin installed, run the setup file again to install additional packages) A fresh install should look something like [this reference image](https://ipfs.io/ipfs/QmaYFSQa4iHDafcebiKjm1WwuKhosoXr45HPpfaeMbCRpb/cygwin%20-%20install.png).

Choose a reason for hiding this comment

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

"...additional packages.) A..."

docs/windows.md Outdated
cd %GOPATH%\src\github.com\ipfs\go-ipfs
make install
```
This can be acheived with the `setx` command
Copy link

@nothingismagick nothingismagick Mar 27, 2018

Choose a reason for hiding this comment

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

"This can be achieved with the setx command: "

docs/windows.md Outdated

**Tip**: You can permenantly append the MSYS2 bin to your `PATH` so that on subsequent builds you can simply run

Choose a reason for hiding this comment

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

permanently

docs/windows.md Outdated
```
git config --global credential.helper wincred
```
If there were no errors, the final command should output version information simillar to "`ipfs version 0.4.14-dev-XXXXXXX`" where "XXXXXXX" should match the current short-hash of the `go-ipfs` repo. You can retrieve said hash via this command: `git rev-parse --short HEAD`. If `ipfs` runs and the version string matches, building was successful.

Choose a reason for hiding this comment

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

"...version information similar to..."

docs/windows.md Outdated

Use the following command to download go-ipfs source code:
**Using build tools manually:**
This section assumes you have a working version of `go` and `git` already setup. You may want to build this way if your environment restricts installing additional software, or if you're intigrating IPFS into your own build system.

Choose a reason for hiding this comment

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

integrating

@nothingismagick
Copy link

@djdv - can't say anything about the content, because windows is not my can of worms - but I did find a massive number of spelling mistakes...

@djdv djdv requested a review from Kubuxu as a code owner March 27, 2018 21:03
@ghost ghost assigned djdv Mar 27, 2018
@ghost ghost added the status/in-progress In progress label Mar 27, 2018
@djdv
Copy link
Contributor Author

djdv commented Mar 27, 2018

@nothingismagick
Thanks!

massive number of spelling mistakes

I've now installed aspell.

Copy link
Contributor

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

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

Oh no, I didn’t realize this was waiting on any further review from me. Sorry!

I think this is looking pretty good. I made a bunch of minor comments I think would be improvements, but none of them are really significant problems except maybe the one about rethinking the “tip” sections for permanently setting paths.

docs/windows.md Outdated
and then install them:
|Command|Explanation|
| ---: | :--- |
|`SET PATH=%PATH%;\msys64\usr\bin` |Add msys2's bin to `PATH`; Defaults to: (\msys64\usr\bin)|
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why I didn’t think of this before, but is it worth pointing out in the explanation that this is what lets pacman and make work?

(Legitimate question, not necessarily a recommendation.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier we say "...we only need MSYS2's tools"
Maybe "Add msys2's bin to PATH" could be reworded to "Add msys2's tools to our PATH" to help imply this? I'll make this change for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like “Add msys2's bin to PATH” and “Add msys2's tools to our PATH” are both equally clear here (although using the same wording in both is good). It was more that, if someone needs explanation for this line, they probably don’t know what it means to add them to “PATH.”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, good point. There should be a reference to https://ss64.com/nt/path.html somewhere here, but I'm not sure where to put it. Either after the phrase or maybe wrapping PATH as a link in that phrase.

docs/windows.md Outdated
|`SET PATH=%PATH%;\msys64\usr\bin` |Add msys2's bin to `PATH`; Defaults to: (\msys64\usr\bin)|
|`pacman --noconfirm -S git make unzip` |Install `go-ipfs` build dependencies|
|`go get -u github.com/ipfs/go-ipfs` |Fetch / Update `go-ipfs` source|
|`cd %GOPATH%\src\github.com\ipfs\go-ipfs` |Change to `go-ipfs` directory|
Copy link
Contributor

Choose a reason for hiding this comment

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

I might say “change to go-ipfs source directory” to clearly connect this with the previous step.

docs/windows.md Outdated

**Tip**: You can permanently append the MSYS2 bin to your `PATH` so that on subsequent builds you can simply run
Copy link
Contributor

Choose a reason for hiding this comment

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

So, now that I’ve looked at this a whole bunch of times, I wonder if it’s confusing that we don’t talk about what you have to do on subsequent builds if you don’t follow this tip. Maybe this section should be:

To build again after making changes to the source, run:

> SET PATH=%PATH%;\msys64\usr\bin
> cd %GOPATH%\src\github.com\ipfs\go-ipfs
> make install

Tip: to avoid adding msys2 to your path every time (SET PATH=%PATH%;\msys64\usr\bin), you can add it permanently using setx:

SETX PATH %PATH%;\msys64\usr\bin

Same for the MSYS2 section.

(Sorry for suggesting a big change here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the way SETX works, I've changed the 'tip' bit a little.
SETX doesn't actually set the variable for the current session, so what we want to do is, first SET PATH and then lock that in with SETX. In addition we don't need to specify the msys|cygwin path since it's already contained in %PATH%, the way it was written before could lead the user to accidentally append it twice if it was already set, now we expect it to be set ahead of time and optionally locked that way.


Tip: To avoid setting PATH every time (SET PATH=%PATH%;\msys64\usr\bin), you can lock it in permanently using setx after it's been set once:

SETX PATH %PATH%

docs/windows.md Outdated
## Cygwin
1. Install Cygwin (https://www.cygwin.com)
2. During the install, select the following packages. (If you already have Cygwin installed, run the setup file again to install additional packages.) A fresh install should look something like [this reference image](https://ipfs.io/ipfs/QmaYFSQa4iHDafcebiKjm1WwuKhosoXr45HPpfaeMbCRpb/cygwin%20-%20install.png).
3.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really part of step 2, I think.

docs/windows.md Outdated
SET PATH=%PATH%;\cygwin64\bin
mkdir %GOPATH%\src\github.com\ipfs
cd %GOPATH%\src\github.com\ipfs
git clone https://github.com/ipfs/go-ipfs.git
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this change? I thought I remembered it being the same as MSYS2, using go get to ensure the right directory structure and clone the git repo. Is there a reason to do it manually for Cygwin and not for MSYS2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tl;dr There's a path conflicts between Cygwin's git and Win32 go. We could more than likely use go get if go was built under Cygwin, but as it is right now they don't provide prebuilts so we just sidestep this by using git directly.


Go has GOPATH restrictions, it wants an absolute native path such as C:\Users\dd\Go.
Cygwin mangles paths to look like this /cygdrive/c/Users/dd/Go.
/ on Windows is a relative path to the current volume's root, so if we set our GOPATH to /cygdrive/c/Users/dd/Go, the go tool refuses to function.

go: GOPATH entry is relative; must be absolute path: "/cygdrive/c/Users/dd/Go".
For more details see: 'go help gopath'

If we set GOPATH to C:\Users\dd\Go, go get will invoke git which receives a mangled path and fails:

# cd .; git clone https://github.com/ipfs/go-ipfs C:\Users\dd\Go\src\github.com\i
pfs\go-ipfs
Cloning into 'C:\Users\dd\Go\src\github.com\ipfs\go-ipfs'...
fatal: Invalid path '/cygdrive/c/Users/dd/C:\Users\dd\Go\src\github.com\ipfs\go-ip
fs': No such file or directory
package github.com/ipfs/go-ipfs: exit status 128

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhhhhhh. That seems like a huge pain; thanks for the explanation. 😕

docs/windows.md Outdated
```
We need the `git` commit hash to be included in our build so that in the extremely rare event a bug is found, we have a reference point later for tracking them. We'll ask `git` for it and store it in a variable. Unfortunately the syntax for the next command is different depending on how it's executed. If you're on the interactive command line vs executing from inside the batch/cmd interpreter. Use the one that applies to you.
Copy link
Contributor

@Mr0grog Mr0grog Mar 30, 2018

Choose a reason for hiding this comment

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

The first sentence needs both “a bug is found” and “tracking them” to be the same singular or plural.

If you're on the interactive command line vs executing from inside the batch/cmd interpreter.

This isn’t a full sentence. It would probably read better combined with the previous sentence, like:

The syntax for the next command is different depending whether you're on the interactive command line or writing a batch script.

docs/windows.md Outdated
```
We need the `git` commit hash to be included in our build so that in the extremely rare event a bug is found, we have a reference point later for tracking them. We'll ask `git` for it and store it in a variable. Unfortunately the syntax for the next command is different depending on how it's executed. If you're on the interactive command line vs executing from inside the batch/cmd interpreter. Use the one that applies to you.
interactive: `FOR /F %V IN ('git rev-parse --short HEAD') do set SHA=%V`
interpreter: `FOR /F %%V IN ('git rev-parse --short HEAD') do set SHA=%%V`
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks fine in plain text, but looks a little confusing when rendered as HTML. Making these a bulleted list would probably help.

docs/windows.md Outdated
```
You can check that the ipfs output versions match with `go version` and `git rev-parse --short HEAD`, if `ipfs.exe` executes and everything matches, then building was successful.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comma after git rev-parse --short HEAD should be either a semicolon or a period.

docs/windows.md Outdated
After that ipfs should have been built and should be available in
"%GOPATH%\bin".
## Troubleshooting
- Git auth
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but I feel like these bullet titles might read better if formatted like:

- **Git auth:** If you get authentication problems…

- **Anything else:** Please search…

docs/windows.md Outdated
```
ipfs version --commit
```
If you encounter a bug with `go-ipfs` itself (not related to building) please use the issue tracker to report it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to make “issue tracker” a link to https://github.com/ipfs/go-ipfs/issues

@djdv
Copy link
Contributor Author

djdv commented Apr 3, 2018

@Mr0grog
No worries, the help is greatly appreciated!

I've updated the draft to reflect some of the changes with minor edits from myself.
15182be

#4691 (comment)
#4691 (comment)
#4691 (comment)

@Mr0grog
Copy link
Contributor

Mr0grog commented Apr 3, 2018

Looks good to me!

License: MIT
Signed-off-by: Dominic Della Valle <ddvpublic@gmail.com>
@djdv
Copy link
Contributor Author

djdv commented Apr 3, 2018

Thanks again for all the help @Mr0grog :^)
Not sure how long this ref will stick around but I made 2 minor changes
a24d1f5
Bold consistency
Tip: -> Tip: (colon is included)

and a references on PATH for the first instance in each section.
PATH -> PATH

@whyrusleeping
Unless there are other remarks by anyone, this may be good to go.

@Mr0grog
Copy link
Contributor

Mr0grog commented Apr 3, 2018

@djdv do you have any idea if the issue in #4905 (git line ending config) is common with Cygwin’s Git? Should we give that a mention in the troubleshooting section?

@djdv
Copy link
Contributor Author

djdv commented Apr 3, 2018

@Mr0grog
When writing this I made sure to go from a fresh Windows install to a built binary using only the steps listed, so by default Cygwins git should work for building, however I didn't test commiting changes with them.

I'll run through the set and see if any of them have CRLF defaults. It wouldn't hurt to have it as supplementary info anyway, I'm just unsure the title. "Go requires Unix-like linefeeds"

@whyrusleeping
Copy link
Member

LGTM, thanks @djdv

@whyrusleeping whyrusleeping merged commit 6321f3b into ipfs:master Apr 24, 2018
@ghost ghost removed the status/in-progress In progress label Apr 24, 2018
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.

4 participants