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

deps: bump go-libedit #38345

Merged
merged 1 commit into from
Jun 21, 2019
Merged

deps: bump go-libedit #38345

merged 1 commit into from
Jun 21, 2019

Conversation

knz
Copy link
Contributor

@knz knz commented Jun 21, 2019

Requested by @benesch in #32623 (comment), needed for bug fixes.

Release note: None

@knz knz requested review from jordanlewis, benesch and a team June 21, 2019 06:36
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

:lgtm: down with sigtramp

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @benesch)

@benesch
Copy link
Contributor

benesch commented Jun 21, 2019

Thanks @knz! Unfortunately it looks like we need to also teach the go-libedit .gitignore file to ignore zcgo_flags files (knz/go-libedit#18). I wrestled with dep for a while but couldn't figure out any other solution. (Without that PR, git status is forever dirtied by the zcgo_flags file we generate into the vendored copy of go-libedit.)

@knz
Copy link
Contributor Author

knz commented Jun 21, 2019

rebasing pr

@benesch
Copy link
Contributor

benesch commented Jun 21, 2019

Ok, so once knz/go-libedit#18 is in place, we need to bump this PR to include that commit and also apply this diff to Gopkg.toml:

diff --git a/Gopkg.toml b/Gopkg.toml
index 456f6c329f..d2455825e8 100644
--- a/Gopkg.toml
+++ b/Gopkg.toml
@@ -25,6 +25,12 @@ ignored = [
   "github.com/google/protobuf/examples/tutorial",
 ]
 
+noverify = [
+  # We generate Go code with cgo configuration flags into go-libedit in order
+  # to link against our own version of libedit.
+  "github.com/knz/go-libedit/unix"
+]
+
 # The collation tables must never change.
 [[constraint]]
   name = "golang.org/x/text"

That ensures that we can generate zcgo_flags files into our vendored copy of go-libedit without causing Git to complain about untracked files, or causing dep to complain that there are extra files in go-libedit.

@benesch
Copy link
Contributor

benesch commented Jun 21, 2019

Woohoo, thanks @knz! Note that you'll need to run bin/dep ensure once more after adding the noverify stanza to Gopkg.toml, since that stanza causes ensure to generate a slightly different Gopkg.lock.

@knz
Copy link
Contributor Author

knz commented Jun 21, 2019

done

@benesch
Copy link
Contributor

benesch commented Jun 21, 2019

Scratch that bit about needing to re-run dep ensure. My patch was just wrong. This is the actual update to Gopkg.toml that's needed:

diff --git a/Gopkg.toml b/Gopkg.toml
index 456f6c329f..7bb3d5483e 100644
--- a/Gopkg.toml
+++ b/Gopkg.toml
@@ -25,6 +25,12 @@ ignored = [
   "github.com/google/protobuf/examples/tutorial",
 ]
 
+noverify = [
+  # We generate Go code with cgo configuration flags into go-libedit in order
+  # to link against our own version of libedit.
+  "github.com/knz/go-libedit"
+]
+
 # The collation tables must never change.
 [[constraint]]
   name = "golang.org/x/text"

Requested by @benesch, needed for bug fixes.

Release note: None
@knz
Copy link
Contributor Author

knz commented Jun 21, 2019

wait how is that different?

@benesch
Copy link
Contributor

benesch commented Jun 21, 2019

It doesn't have the trailing /unix in the package name. I guess noverify takes only "project roots", not packages. This is all so confusing.

Anyway, not to worry! I still have the commit bit here apparently so I was able to update the patch accordingly. 🤞

Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

LGTM if it LGTY, @knz!

@knz
Copy link
Contributor Author

knz commented Jun 21, 2019

let's fly!

bors r+

craig bot pushed a commit that referenced this pull request Jun 21, 2019
38345: deps: bump go-libedit r=knz a=knz

Requested by @benesch in #32623 (comment), needed for bug fixes.

Release note: None

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jun 21, 2019

Build succeeded

@craig craig bot merged commit d89efca into cockroachdb:master Jun 21, 2019
@knz knz deleted the 20190620-libedit branch June 21, 2019 20:18
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