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

Fix misleading README install instructions. #7427

Merged
merged 1 commit into from Jun 27, 2017

Conversation

ExternalReality
Copy link
Contributor

Description of change

Fixes the referenced bug.

  • The README did not mention what version of Go is required to build the source
  • The top-level Makefile installed Go as a dependency but Go may already be installed by other means leading to confusing situations when coupled with the issue above

QA steps

Follow the README instructions. They should work the same with the installation of Go happening as an explicit step.

Documentation changes

N/A

Bug reference

https://bugs.launchpad.net/juju/+bug/1662857

README.md Outdated
--------------

`juju` currently depends on Go 1.8. One of the easiest ways to install golang is
by [getting snap](https://snapcraft.io/docs/core/install) and then installing the snap package
Copy link
Member

Choose a reason for hiding this comment

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

"getting snap" reads a little funny.
Maybe something along the lines of:

... to install golang is from a snap. You may need to first install the snap client. Installing the golang snap package is then as easy as:

snap install go --classic

README.md Outdated

You can read about the "classic" confinement policy [here](https://insights.ubuntu.com/2017/01/09/how-to-snap-introducing-classic-confinement/)

If you want to use `apt`, then you can add [juju-golang PPA](https://launchpad.net/~juju/+archive/ubuntu/golang) and then run the following
Copy link
Member

Choose a reason for hiding this comment

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

can add the

@ExternalReality ExternalReality force-pushed the bug_1662857 branch 2 times, most recently from f80cd77 to 0613b99 Compare June 1, 2017 02:37
@@ -98,7 +89,6 @@ rebuild-dependencies.tsv: godeps
# PPA includes the required mongodb-server binaries.
install-dependencies:
@echo Adding juju PPAs for golang and juju
@sudo apt-add-repository --yes ppa:juju/golang
Copy link
Member

Choose a reason for hiding this comment

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

given 'make check' is how some of the CI jobs work, are we sure this won't actually cause problems on non-developer machines?

@nskaggs
Copy link
Contributor

nskaggs commented Jun 5, 2017

@ExternalReality do you need help in landing this still?

Copy link

@mjs mjs left a comment

Choose a reason for hiding this comment

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

It would be good to get this into the 2.2 branch as well so that people aren't confused when trying to build that.

@@ -98,7 +89,6 @@ rebuild-dependencies.tsv: godeps
# PPA includes the required mongodb-server binaries.
install-dependencies:
@echo Adding juju PPAs for golang and juju
Copy link

Choose a reason for hiding this comment

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

I think install-dependencies should also do this:

        go get -u github.com/rogpeppe/godeps
        $(GOPATH)/bin/godeps -u dependencies.tsv

Anyone who is wanting to build Juju is going to need godeps and grab the dependencies so I think the build target should do it for them.

INSTALL_FLAGS :=
else
GO_C := gccgo-4.9 gccgo-go
INSTALL_FLAGS := -gccgoflags=-static-libgo
Copy link

Choose a reason for hiding this comment

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

INSTALL_FLAGS is still referenced elsewhere. I think only GO_C can be removed.

README.md Outdated
@@ -22,6 +22,24 @@ stable PPA, `https://launchpad.net/~juju/+archive/stable`, and can be installed
sudo apt-add-repository ppa:juju/stable
sudo apt-get update
sudo apt-get install juju

Getting Golang
Copy link

Choose a reason for hiding this comment

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

How about "Installing Go"?

README.md Outdated
Getting Golang
--------------

`juju` source code currently depends on Go 1.8. One of the easiest ways
Copy link

Choose a reason for hiding this comment

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

"Juju's source code..."


sudo apt install golang-1.8

Alternatively, you can always follow the official [binary installation instructions](https://golang.org/doc/install#install)
Copy link

Choose a reason for hiding this comment

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

This new text is great overall. Thanks.

@ExternalReality ExternalReality force-pushed the bug_1662857 branch 3 times, most recently from 55ddf55 to 679841d Compare June 26, 2017 22:00
@ExternalReality
Copy link
Contributor Author

!!build!!

@ExternalReality
Copy link
Contributor Author

$$MERGE$$

@jujubot
Copy link
Collaborator

jujubot commented Jun 27, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 433beea into juju:develop Jun 27, 2017
@ExternalReality ExternalReality deleted the bug_1662857 branch June 27, 2017 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants