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.

Sign up for GitHub

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

Jump to bottom

cockroachdb: refactor and 1.1.5->2.0.0 #38968

Closed
wants to merge 1 commit into from
Closed

cockroachdb: refactor and 1.1.5->2.0.0 #38968

wants to merge 1 commit into from

Conversation

bdarnell
Copy link
Contributor

Motivation for this change

Building CockroachDB from a tarball has little in common with other go
packages, so buildGoPackage wasn't doing anything useful. Refactor to
a plain derivation with go as a dependency. Build from top-level
instead of in the go package path.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@bdarnell
cockroachdb: refactor and 1.1.5->2.0.0
4b04a5c 
Building CockroachDB from a tarball has little in common with other go
packages, so buildGoPackage wasn't doing anything useful. Refactor to
a plain derivation with go as a dependency. Build from top-level
instead of in the go package path.
@bdarnell
Copy link
Contributor Author

This PR is offered as an alternative to #38662.

@jbboehr
Copy link
Contributor

jbboehr commented Apr 15, 2018

@bdarnell I believe nativeBuildInputs is used to refer to packages that are not required at run-time (compilers and the like), so unless whatever libedit provides is only required during build, I believe it should be added to buildInputs.

Mic92
Mic92 reviewed Apr 15, 2018
View reviewed changes
pkgs/servers/sql/cockroachdb/default.nix

buildGoPackage rec {
stdenv.mkDerivation rec {
Copy link
Member

Choose a reason for hiding this comment

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

Please stick to buildGoPackage even if the build phase is overwritten. Otherwise cockroach will have runtime references to the go compiler, which should be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, OK. In that case #38662 should be preferred, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that it's not just go - only ncurses/libedit are needed at runtime; all the other deps (cmake, autoconf, etc) are build-time-only. Ideally those would also be filtered out the same way that buildGoPackage does for the go compiler.

Copy link
Member

Choose a reason for hiding this comment

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

This is detected by nix. The problem with go is that string references in binaries are embedded and therefore create a runtime dependency on the compiler.

jbboehr added a commit to jbboehr/nixpkgs that referenced this pull request Apr 15, 2018
@jbboehr
Incorporate libedit changes from NixOS#38968
4707e69
@bdarnell bdarnell closed this Apr 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bdarnell @jbboehr @Mic92 @GrahamcOfBorg