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

credhub-cli: fix build under go1.15 #107940

Merged
merged 1 commit into from May 1, 2021
Merged

credhub-cli: fix build under go1.15 #107940

merged 1 commit into from May 1, 2021

Conversation

c00w
Copy link
Contributor

@c00w c00w commented Dec 30, 2020 •
edited

I've sent this upstream at
cloudfoundry/credhub-cli#107

But this should fix the build so it doesn't require go1.14

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested review from kalbasit and risicle December 30, 2020 05:23
risicle
risicle approved these changes Dec 30, 2020
View reviewed changes
Copy link
Contributor

@risicle risicle left a comment

Choose a reason for hiding this comment

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

Nice one. WFM macos 10.14 & non-nixos linux x86_64

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 107940 run on x86_64-linux 1

1 package built:
  • credhub-cli

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 107940 run on x86_64-darwin 1

1 package failed to build:
  • credhub-cli

Tests fail due to sandbox.

@risicle
Copy link
Contributor

risicle commented Dec 30, 2020

Tests fail due to sandbox.

Was this not fixed by __darwinAllowLocalNetworking?

@ofborg ofborg bot requested a review from risicle December 31, 2020 21:13
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review. If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 107940 run on x86_64-linux 1

1 package built:
  • credhub-cli

@SuperSandro2000
Copy link
Member

Tests fail due to sandbox.

Was this not fixed by __darwinAllowLocalNetworking?

yes

@c00w
Copy link
Contributor Author

c00w commented Jan 1, 2021

rebased on master since it sounds like the macos failures are already fixed - I don't have (and likely never will have) a mac and thus cannot debug :|

@c00w
Copy link
Contributor Author

c00w commented Jan 6, 2021

@SuperSandro2000 - any idea how to rekick ofborg?

@SuperSandro2000
Copy link
Member

@ofborg eval

@SuperSandro2000
Copy link
Member

rebased on master since it sounds like the macos failures are already fixed - I don't have (and likely never will have) a mac and thus cannot debug :|

Rebasing against master is usually not required.

@SuperSandro2000
Copy link
Member

@c00w please fix the merge conflict.

@c00w
Copy link
Contributor Author

c00w commented Jan 14, 2021

Rebased on a newer master to fix merge conflicts.

@c00w
Copy link
Contributor Author

c00w commented Jan 14, 2021

@SuperSandro2000 ^^

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 107940 run on x86_64-darwin 1

1 package failed to build and are new build failure:

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 107940 run on x86_64-linux 1

1 package built:
  • credhub-cli

@c00w
Copy link
Contributor Author

c00w commented May 1, 2021

Any update - since this PR was opened we've gotten a new go version that also needs patches - we're failing to keep up with go releases :P

@c00w
credhub-cli: fix build under go1.15
36a44c4 
I've sent this upstream at
cloudfoundry/credhub-cli#107

But this should fix the build so it doesn't require go1.14
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/510

@risicle
Copy link
Contributor

risicle commented May 1, 2021

I think what we're missing here is a darwin tester who can check the build with and without this change on the same machine for us.

@SuperSandro2000 SuperSandro2000 merged commit 783f5cd into NixOS:master May 1, 2021
@SuperSandro2000
Copy link
Member

We can fix the darwin issue later if we found someone on darwin to test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested changes

@zowoq zowoq zowoq left review comments

@kalbasit kalbasit Awaiting requested review from kalbasit

@risicle risicle Awaiting requested review from risicle

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@c00w @SuperSandro2000 @risicle @nixos-discourse @zowoq