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

ipfs-cluster: Remove go1.14 pinning. #107941

Merged
merged 1 commit into from Dec 30, 2020
Merged

ipfs-cluster: Remove go1.14 pinning. #107941

merged 1 commit into from Dec 30, 2020

Conversation

c00w
Copy link
Contributor

@c00w c00w commented Dec 30, 2020 •
edited

It builds without it, so I think it's just some legacy cruft

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.

@c00w
ipfs-cluster: Remove go1.14 pinning.
f7fc3bf 
It builds without it, so I think it's just some legacy cruft
@SuperSandro2000
Copy link
Member

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

1 package built:
  • ipfs-cluster

@prusnak prusnak merged commit bee0282 into NixOS:master Dec 30, 2020
@SuperSandro2000
Copy link
Member

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

1 package built:
  • ipfs-cluster

@bqv
Copy link
Contributor

bqv commented Jan 8, 2021

Ah, so this is why it broke for me.

@bqv
Copy link
Contributor

bqv commented Jan 8, 2021 •
edited

If you're going to remove the 1.14 pin, you'll need to bump the version by a few commits, or it builds but doesn't actually work. Maybe add an installCheck so nobody does something like this again (something as simple as installCheckPhase = "$out/bin/ipfs-cluster-service --help"; woulda caught this)

alyssais added a commit that referenced this pull request Jan 8, 2021
@alyssais
Revert "ipfs-cluster: Remove go1.14 pinning."
ca90852 
Quoting <#107941 (comment)>:

> If you're going to remove the 1.14 pin, you'll need to bump the
> version by a few commits, or it builds but doesn't actually work.

This reverts commit f7fc3bf.
@c00w
Copy link
Contributor Author

c00w commented Jan 8, 2021

I'm suprised there aren't some tests running for ipfs - I'll see if I can at least enable some tests this weekend.

@c00w
Copy link
Contributor Author

c00w commented Jan 8, 2021

@bqv - can you include what's failing? Testing this on my machine it's working on head.

~/nixpkgs % git show HEAD                                                                                                                                               ~/nixpkgs 
commit 9ff1aa8d2ce70fa2c4417eab055d182623f98429 (HEAD -> master, origin/master, origin/HEAD)                                                                                                   
Merge: 964c419bc0e 952df7049b4                                                                                                                                                                 
Author: Pavol Rusnak <pavol@rusnak.io>                                                                                                                                                         
Date:   Sat Jan 9 00:25:55 2021 +0100                                                                                                                                                          
                                                                                                                                                                                               
    Merge pull request #106888 from NomisIV/master                                                                                                                                             
     
    blender: add option for OptiX

 ~/nixpkgs % nix-build default.nix -A ipfs                                                                                                                               ~/nixpkgs
/nix/store/acsvh9rkv21xsbhb00bmy14gf15vg1sr-ipfs-0.7.0
 ~/nixpkgs % $(nix-build default.nix -A ipfs)/bin/ipfs --help | head                                                                                                     ~/nixpkgs
USAGE
  ipfs  - Global p2p merkle-dag filesystem.

SYNOPSIS
  ipfs [--config=<config> | -c] [--debug | -D] [--help] [-h] [--api=<api>] [--offline] [--cid-base=<base>] [--upgrade-cidv0-in-output] [--encoding=<encoding> | --enc] [--timeout=<timeout>] <command> ...

OPTIONS

  -c, --config               string - Path to the configuration file to use.
  -D, --debug                bool   - Operate in debug mode.
~/nixpkgs % echo $?                                                                                                                                                     ~/nixpkgs
0

@bqv
Copy link
Contributor

bqv commented Jan 8, 2021

I did include what was failing in my previous comment

@bqv
Copy link
Contributor

bqv commented Jan 8, 2021

N.B. Your PR has been reverted on HEAD now

@c00w
Copy link
Contributor Author

c00w commented Jan 8, 2021

@bqv ah - ipfs-cluster package, for some reason I thought this was ipfs :) very confused.

@c00w
Copy link
Contributor Author

c00w commented Jan 8, 2021

And the tests for ipfs-cluster are disabled facepalm

@bqv
Copy link
Contributor

bqv commented Jan 8, 2021

Don't enable those checks either. They're disabled for a reason - they spend 10 minutes doing nothing and then fail due to no network. If you're going to do anything, add an installcheck as I suggested

@c00w
Copy link
Contributor Author

c00w commented Jan 9, 2021

@bqv those tests pass on my machine (except for 1). Eyeballing my nix.conf, I do have sandboxing enabled, so they seem good.

@c00w
Copy link
Contributor Author

c00w commented Jan 9, 2021

#108822

@bqv
Copy link
Contributor

bqv commented Jan 9, 2021

My bad, you are right on that one. In fact, on my overlay where I've updated to the latest commit, all tests pass (albeit after 15 minutes)

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

@prusnak prusnak prusnak approved these changes

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
4 participants
@c00w @SuperSandro2000 @bqv @prusnak