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: configure and build patched libedit #32623

Merged
merged 1 commit into from
Jun 21, 2019
Merged

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Nov 27, 2018

There is more work to do here to appease the linter, but wanted to get this proof-of-concept out. @knz what copyright header would you like me to put on the files in cli/editline?


Absorb our line-editing library, github.com/knz/go-libedit. The C
library, libedit, is extracted into a c-deps submodule, where its build
can be properly orchestrated by our Makefile to use the upstream
autotools build system. The Go code is extracted into pkg/cli/editline,
where it can more easily be modified to meet our needs.

Fix #31843.

Release note: None


Teach our Makefile to orchestrate the configuration and building of our
line editing library, libedit. This allows us to incorporate an upstream
patch for #31843, which was causing incorrect spacing when recalling
multi-line history entries. Previously, we were attempting to link
against potentially outdated system libedits, which are likely to
exhibit the bug for years to come.

Release note (build change): a recent libedit is now bundled with
CockroachDB, which fixes some line editing bugs in the CockroachDB
console. On platforms that include libedit as part of the base system,
like macOS and FreeBSD, CockroachDB no longer links against the system
libedit.

@benesch benesch requested review from jordanlewis, bdarnell, knz and a team November 27, 2018 05:33
@benesch benesch requested a review from a team as a code owner November 27, 2018 05:33
@benesch benesch requested a review from a team November 27, 2018 05:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Nov 27, 2018

  1. You're defining a fork since the implementation is (slightly) different.

  2. I'm sorry but you can't just add copyright headers using the APL license terms and call it a day. the proper course of action is:

  • for the sources that are specific to go-libedit:

    • add a header that mentions the proper license terms (go-libedit's BSD terms; you can take inspiration from the c libedit files)
    • mention the original copyright at the top of the license terms
    • add the cockroachdb copyright on top of those specific files you're modifying
    • change the linter to accept all that
  • for the sources imported from libedit:

    • leave the headers that are there as-is
  • mention these licenses in the top level licenses/ directory

@benesch benesch changed the title [wip] cli: absorb github.com/knz/go-libedit cli: absorb and fork github.com/knz/go-libedit Nov 27, 2018
@benesch
Copy link
Contributor Author

benesch commented Nov 27, 2018

Ok, I think this is ready to go. @knz I added your copyright header to all of the files that originated from github.com/knz/go-libedit. I'm not seeing why the NetBSD copyright would be relevant to any of them; they're all your original work, save for a function or two that I contributed.

The copyright on libedit itself, of course, very much belongs to NetBSD and Christos Zoulas. We don't need to worry about that, though, since it's isolated in its own Git submodule with appropriate license files and headers.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

You can't just add "portions of this file are with a different license"

This is not how licenses work.

The original code was released with a specific license. That license grants you right that you would otherwise not have since you are not copyright holder.

The license grants you the following rights:

  • make copies and redistribution of the files
  • make changes to the file, holding the copyright and license info intact
  • claim copyright to portions of the files

Here is a right that the license does not grant you:

  • issue a new license for the files

Licenses do not apply for a "portion" of the file. Licenses apply on entire files.

Also, you may not issue a license for code for which you are not the sole copyright owner, without agreement from every other copyright holder.

If you want to have a mixed-license source tree you must be very careful to:

  • separate code with different copyright holders into different files, with clear copyright attributions
  • license the files that you have sole copyright over using a a separate license, if you wish to do so
  • identify clearly (usually, by using separate directories) which files are released with which license.

@knz
Copy link
Contributor

knz commented Nov 27, 2018

So in terms of changes:

  • if you wish to edit files from go-libedit, you can do so and add the cockroach name, but not change the licensing terms (and not "portions of this file with separate license" - that doesn not work)

  • if you insist on using APL for your changes, make these changes on different files and use #include / #ifdef to make them work the way you want

@benesch
Copy link
Contributor Author

benesch commented Nov 27, 2018

We do the "portions of this file" thing elsewhere, so if that is indeed wrong there is a lot to clean up:

// Copyright 2011 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in licenses/BSD-golang.txt.
// Portions of this file are additionally subject to the following
// license and copyright.
//
// Copyright 2016 The Cockroach Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
// implied. See the License for the specific language governing
// permissions and limitations under the License.
// Copied from Go's text/template/parse package and modified for yacc.

Here is a right that the license does not grant you:

issue a new license for the files

Why not? The derivative work is subject to the terms of both licenses: your original BSD license as well as the Apache license.

Licenses do not apply for a "portion" of the file. Licenses apply on entire files.

Source? I think this is just a convention.

I don't have a problem with releasing my changes in this directory under the terms of your BSD license, but I think doing so requires @bdarnell's permission.

@knz
Copy link
Contributor

knz commented Nov 27, 2018

This discussion is above my paygrade but all my experience until now has been that open source software avoid separate licensing terms for separate portions of files.

This is because it makes tracking attribution and licensing of individual lines/words inside the file properly impossible.

I do indeed think that we have a serious problem on our hands if that has been done in other places. I'd like to see some serious precedent outside of CockroachDB before I build a different opinion.

@benesch benesch force-pushed the libedit branch 8 times, most recently from 9bd423e to 9f5086b Compare November 27, 2018 21:38
@knz
Copy link
Contributor

knz commented Nov 27, 2018

this was discussed with @bdarnell. please issue first a PR against go-libedit to re-license with APL; we'll merge that, then you can pick it up from there.

@knz
Copy link
Contributor

knz commented Jun 21, 2019

well thanks to you kind sir

Teach our Makefile to orchestrate the configuration and building of our
line editing library, libedit. This allows us to incorporate an upstream
patch for cockroachdb#31843, which was causing incorrect spacing when recalling
multi-line history entries. Previously, we were attempting to link
against potentially outdated system libedits, which are likely to
exhibit the bug for years to come.

Release note (build change): a recent libedit is now bundled with
CockroachDB, which fixes some line editing bugs in the CockroachDB
console. On platforms that include libedit as part of the base system,
like macOS and FreeBSD, CockroachDB no longer links against the system
libedit.
@benesch
Copy link
Contributor Author

benesch commented Jun 21, 2019

It's green! 💚

bors r=knz

craig bot pushed a commit that referenced this pull request Jun 21, 2019
32623: deps: configure and build patched libedit r=knz a=benesch

~There is more work to do here to appease the linter, but wanted to get this proof-of-concept out. @knz what copyright header would you like me to put on the files in cli/editline?~

---

~Absorb our line-editing library, github.com/knz/go-libedit. The C
library, libedit, is extracted into a c-deps submodule, where its build
can be properly orchestrated by our Makefile to use the upstream
autotools build system. The Go code is extracted into pkg/cli/editline,
where it can more easily be modified to meet our needs.~

~Fix #31843.~

~Release note: None~

---

Teach our Makefile to orchestrate the configuration and building of our
line editing library, libedit. This allows us to incorporate an upstream
patch for #31843, which was causing incorrect spacing when recalling
multi-line history entries. Previously, we were attempting to link
against potentially outdated system libedits, which are likely to
exhibit the bug for years to come.

Release note (build change): a recent libedit is now bundled with
CockroachDB, which fixes some line editing bugs in the CockroachDB
console. On platforms that include libedit as part of the base system,
like macOS and FreeBSD, CockroachDB no longer links against the system
libedit.

Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
@craig
Copy link
Contributor

craig bot commented Jun 21, 2019

Build succeeded

@craig craig bot merged commit 454f32b into cockroachdb:master Jun 21, 2019
@benesch benesch deleted the libedit branch June 21, 2019 21:59
@bobvawter
Copy link
Member

This patch appears to break the nightly Windows build

[01:57:30]2019/06/24 01:57:30 [] [mkrelease windows GOFLAGS= SUFFIX=.windows-6.2-amd64.exe TAGS= BUILDCHANNEL=official-binary]
[01:57:30]+ CGO_ENABLED=1
[01:57:30]+ make BUILDTYPE=release XGOOS=windows XGOARCH=amd64 XCMAKE_SYSTEM_NAME=Windows TARGET_TRIPLE=x86_64-w64-mingw32 LDFLAGS=-static SUFFIX=-windows-6.2-amd64 GOFLAGS= SUFFIX=.windows-6.2-amd64.exe TAGS= BUILDCHANNEL=official-binary
[01:57:32]configure: error: libncurses, libcurses, or libtermcap is required!
[01:57:33]make: *** [/go/native/x86_64-w64-mingw32/libedit/Makefile] Error 1
[01:57:33]make: *** Waiting for unfinished jobs....
[01:57:58]2019/06/24 01:57:58 [mkrelease windows GOFLAGS= SUFFIX=.windows-6.2-amd64.exe TAGS= BUILDCHANNEL=official-binary]: exit status 1

@benesch
Copy link
Contributor Author

benesch commented Jun 24, 2019

Oh, hm, I guess we shouldn't bother trying to build libedit for Windows because it certainly won't work there.

@benesch
Copy link
Contributor Author

benesch commented Jun 24, 2019

I'll have a patch for this shortly.

@danhhz
Copy link
Contributor

danhhz commented Jun 24, 2019

Awesome! Feel free to cc me on the review. I'm about to cut a 19.2 alpha and this has Publish Bleeding Edge blocked

(Hi Nikhil! 👋)

danhhz added a commit to danhhz/cockroach that referenced this pull request Jun 24, 2019
This reverts commit 454f32b. It breaks
the Publish Bleeding Edge build, which blocks releases, nightly tests,
and roachperf data collection.

Looks like the failure is that it's trying to build libedit on windows:
cockroachdb#32623 (comment)

Release note: None
craig bot pushed a commit that referenced this pull request Jun 24, 2019
38383: Revert "deps: configure and build patched libedit" r=jordanlewis a=danhhz

This reverts commit 454f32b. It breaks
the Publish Bleeding Edge build, which blocks releases, nightly tests,
and roachperf data collection.

Looks like the failure is that it's trying to build libedit on windows:
#32623 (comment)

Release note: None

Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
@danhhz
Copy link
Contributor

danhhz commented Jun 24, 2019

Argh, sorry, but I had to revert this to get an alpha release started 😬. We didn't have any recent green Publish Bleeding Edge builds. #38383. Should be easy to unrevert with the windows fix, tho

@benesch
Copy link
Contributor Author

benesch commented Jun 24, 2019 via email

benesch added a commit to benesch/cockroach that referenced this pull request Jun 25, 2019
Libedit doesn't support Windows, so don't try to build/link it there.
This was the previous behavior, but I broke it in cockroachdb#32623.

Release note: None
craig bot pushed a commit that referenced this pull request Jun 25, 2019
38400: deps: configure and build patched libedit redux r=jordanlewis a=benesch

Resubmit #32623 with a patch to avoid building libedit on Windows, where it's unsupported. I've verified that `build/builder.sh mkrelease windows` succeeds with this additional patch.

Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-noremind Bots won't notify about PRs with X-noremind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli: multi-line history replay produces terminal output with padded spaces instead of newlines
8 participants