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

build: make 'go get github.com/cockroachdb/cockroach' fail #25325

Merged
merged 1 commit into from
May 30, 2018

Conversation

benesch
Copy link
Contributor

@benesch benesch commented May 4, 2018

The root package, github.com/cockroachdb/cockroach, previously served as
the main entry point for the default CCL binary. This meant that naively
running

$ go get github.com/cockroachdb/cockroach

would fail with cryptic errors about missing "reserved keywords", as our
build requires generating some Go code via Make. Our documentation
suggests that users instead run

$ go get -d github.com/cockroachdb/cockroach
$ make

but many users, somewhat understandably, fail to read that
documentation.

Move the main entry point to ./pkg/cmd/cockroach. Running 'go get
github.com/cockroachdb/cockroach' will now produce a less confusing
error that reads:

can't load package: package github.com/cockroachdb/cockroach: no Go
files in GOPATH/src/github.com/cockroachdb/cockroach

Hopefully this spurs users to read the build instructions.

(Running 'go get github.com/cockroachdb/cockroach/pkg/cmd/cockroach'
still produces the same cryptic error messages as before, but users are
far less likely to construct that 'go get' invocation.)

Fix #23583.
Fix #23992.

Release note: None

@benesch benesch requested review from bdarnell, knz and a team May 4, 2018 21:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

This may warrant a PSA on the mailing list, because I think if you've set the "scope" for go-guru in your editor to github.com/cockroachdb/cockroach you'll need to update it to point to the new root. (although I'm apparently wrong about how this works, because I don't have go-guru-scope set in my editor currently. I thought it was required)

The root package, github.com/cockroachdb/cockroach, previously served as
the main entry point for the default CCL binary. This meant that naively
running

    $ go get github.com/cockroachdb/cockroach

would fail with cryptic errors about missing "reserved keywords", as our
build requires generating some Go code via Make. Our documentation
suggests that users instead run

    $ go get -d github.com/cockroachdb/cockroach
    $ make

but many users, somewhat understandably, fail to read that
documentation.

Move the main entry point to ./pkg/cmd/cockroach. Running 'go get
github.com/cockroachdb/cockroach' will now produce a less confusing
error that reads:

    can't load package: package github.com/cockroachdb/cockroach: no Go
    files in GOPATH/src/github.com/cockroachdb/cockroach

Hopefully this spurs users to read the build instructions.

(Running 'go get github.com/cockroachdb/cockroach/pkg/cmd/cockroach'
still produces the same cryptic error messages as before, but users are
far less likely to construct that 'go get' invocation.)

Fix cockroachdb#23583.

Release note: None
@benesch benesch requested a review from a team as a code owner May 11, 2018 17:01
@benesch
Copy link
Contributor Author

benesch commented May 11, 2018

@knz I've been holding off on merging this because I'll be on vacation for the next two weeks and won't have time to address the fallout. If you're willing to take responsibility, though, please feel free to notify the mailing list and merge!

@benesch
Copy link
Contributor Author

benesch commented May 30, 2018

bors r=bdarnell

craig bot pushed a commit that referenced this pull request May 30, 2018
25325: build: make 'go get github.com/cockroachdb/cockroach' fail r=bdarnell a=benesch

The root package, github.com/cockroachdb/cockroach, previously served as
the main entry point for the default CCL binary. This meant that naively
running

    $ go get github.com/cockroachdb/cockroach

would fail with cryptic errors about missing "reserved keywords", as our
build requires generating some Go code via Make. Our documentation
suggests that users instead run

    $ go get -d github.com/cockroachdb/cockroach
    $ make

but many users, somewhat understandably, fail to read that
documentation.

Move the main entry point to ./pkg/cmd/cockroach. Running 'go get
github.com/cockroachdb/cockroach' will now produce a less confusing
error that reads:

    can't load package: package github.com/cockroachdb/cockroach: no Go
    files in GOPATH/src/github.com/cockroachdb/cockroach

Hopefully this spurs users to read the build instructions.

(Running 'go get github.com/cockroachdb/cockroach/pkg/cmd/cockroach'
still produces the same cryptic error messages as before, but users are
far less likely to construct that 'go get' invocation.)

Fix #23583.
Fix #23992.

Release note: None

Co-authored-by: Nikhil Benesch <[email protected]>
@craig
Copy link
Contributor

craig bot commented May 30, 2018

Build succeeded

@craig craig bot merged commit e64d40e into cockroachdb:master May 30, 2018
@rendaw
Copy link

rendaw commented May 30, 2018

So, to get the fallout started:

The docs still say

2. Get the CockroachDB code:
go get -d github.com/cockroachdb/cockroach
cd $(go env GOPATH)/src/github.com/cockroachdb/cockroach

but the first step fails (just broke an automated build I'm working on).

What should one do now? mkdir -p /go/src/github.com/cockroachdb && git clone https://github.com/cockroachdb/cockroach.git /go/src/github.com/cockroachdb/cockroach?

As an additional note, as go get doesn't support version pinning so any scripts that used that (even if they eventually built off a tag) have probably been broken by this. Not sure how widespread that practice is though.

benesch added a commit to benesch/cockroach that referenced this pull request May 30, 2018
Since cockroachdb#25325 landed, 'go get' no longer works. Adjust our contributing
guidelines appropriately.

Release note: None
craig bot pushed a commit that referenced this pull request May 31, 2018
26239: CONTRIBUTING.md: avoid recommendation to use 'go get' r=couchand a=benesch

Since #25325 landed, 'go get' no longer works. Adjust our contributing
guidelines appropriately.

Release note: None

/cc @rendaw (thanks for the report!)

26290: sql, opt: Add pg error codes, tests for error messages r=rytaft a=rytaft

This commit ensures that all errors in the optbuilder have a pg
errorcode. In order to keep the optimizer in sync with the heuristic
planner, this commit also updates some of the heuristic planner error
messages to match the Postgres error messages and include pgcodes.

Other errors in the optbuilder which should never be thrown (e.g., in
the default case of a switch statement that covers all cases) have been
converted into panics.

Release note (sql change): Fixed some error messages to more closely
match the Postgres error messages, including the corresponding Postgres
error codes.

26296: importccl: re-enable TestImportCSVStmt r=mjibson a=mjibson

The underlying bug appears to have been fixed in #26259.

Release note: None

26298: Makefile: fix check-libroach with linux release toolchain r=mberhault a=benesch

The Linux release toolchain uses an ancient version of glibc that
requires linking against -lrt. Teach CMake to link test binaries
accordingly on Linux.

The Linux release toolchain also uses a very recent version of libstdc++
that must be statically linked. Plumb the link flags down into CMake via
the LDFLAGS variable. (Previously they were only passed down to links
invoked by Go via the -extldflags flag in the LINKFLAGS variable.)

Release note: None

Co-authored-by: Nikhil Benesch <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>
Co-authored-by: Matt Jibson <[email protected]>
@benesch benesch deleted the no-go-get branch June 6, 2018 16:42
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