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

tahoe-lafs: clean up inputs #59231

Merged
merged 5 commits into from Apr 11, 2019
Merged

tahoe-lafs: clean up inputs #59231

merged 5 commits into from Apr 11, 2019

Conversation

exarkun
Copy link
Contributor

@exarkun exarkun commented Apr 9, 2019

Motivation for this change

make tahoe-lafs build on aarch64

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

numpy has not been a dependency since 2012.
apart from being unnecessary, numpy depends on openblas which fails to build on aarch64.

wheel seems to be a dependency.  without it "error: invalid command 'bdist_wheel'"
@exarkun
Copy link
Contributor Author

exarkun commented Apr 9, 2019

@MostAwesomeDude

@flokli flokli changed the title fix build on aarch64 tahoe-lafs: fix build on aarch64 Apr 9, 2019
We need it at build time.
It is only used for the test suite.
It is no longer used at all.
@dotlambda
Copy link
Member

@GrahamcOfBorg build tahoe-lafs

@dotlambda
Copy link
Member

Tahoe-lafs seems to build fine on the aarch64 Hydra machine: https://hydra.nixos.org/build/91870347
You're sure this change is needed?

@exarkun
Copy link
Contributor Author

exarkun commented Apr 10, 2019

Tahoe-lafs seems to build fine on the aarch64 Hydra machine: https://hydra.nixos.org/build/91870347
You're sure this change is needed?

I'm sure of very little. But the build clearly fails in my environment.

@exarkun
Copy link
Contributor Author

exarkun commented Apr 10, 2019

Tahoe-lafs seems to build fine on the aarch64 Hydra machine: https://hydra.nixos.org/build/91870347
You're sure this change is needed?

I'm sure of very little. But the build clearly fails in my environment.

Maybe I should make it more clear that this build happens in the context of building an aarch64 sd-image? Could that make some difference? Like ... the normal/hydra build drags in the wheel dependency by accident somehow so the build works but without the explicit declaration it fails when the sd-image build fails to provide the dependency?

I don't know, wildly guessing here.

@dotlambda
Copy link
Member

dotlambda commented Apr 10, 2019

Could that make some difference?

I wouldn't think so. I suggest you run nix-build -A tahoe-lafs and check if that works.
Which git revision of nixpkgs are you using?

@exarkun
Copy link
Contributor Author

exarkun commented Apr 10, 2019

Which git revision of nixpkgs are you using?

Using nixpkgs-unstable channel. This seems to be at 19.09pre174594.0c0954781e2

I wouldn't think so. I suggest you run nix-build -A tahoe-lafs and check if that works.

I got a checkout of nixpkgs and ran this. It succeeds just by fetching from the binary cache. Not sure how to force it to try actually building locally.

@exarkun
Copy link
Contributor Author

exarkun commented Apr 10, 2019

Cannot reproduce the failure on nixpkgs master@HEAD.

@exarkun exarkun closed this Apr 10, 2019
@exarkun exarkun deleted the patch-1 branch April 10, 2019 17:31
@flokli
Copy link
Contributor

flokli commented Apr 11, 2019

@dotlambda, @exarkun do still want to include the cleanups from here?

@dotlambda
Copy link
Member

dotlambda commented Apr 11, 2019

do still want to include the cleanups from here?

Yes, that would be nice.

@dotlambda
Copy link
Member

Not sure how to force it to try actually building locally.

You can use --check.

@exarkun
Copy link
Contributor Author

exarkun commented Apr 11, 2019

Maybe I should suggest these cleanups for #59258 ?

@dotlambda
Copy link
Member

You can simply repon this PR or open a new one. Just remove wheel from the expression.

@exarkun exarkun restored the patch-1 branch April 11, 2019 16:29
This is no longer (was never? unclear) required.
@exarkun
Copy link
Contributor Author

exarkun commented Apr 11, 2019

Okay. Here's an update that only:

  • drops unzip and numpy buildInputs
  • moves check-only mock input to checkInputs

I verified that this builds (thus, check passes) but I didn't try running the software yet.

@exarkun exarkun reopened this Apr 11, 2019
@exarkun exarkun changed the title tahoe-lafs: fix build on aarch64 tahoe-lafs: clean up inputs Apr 11, 2019
@exarkun
Copy link
Contributor Author

exarkun commented Apr 11, 2019

I've now done a few basic manual tests of the output (the "tahoe" program) and it seems good.

@dotlambda
Copy link
Member

Maybe we should keep numpy in propagatedBuildInputs? Seems to be an optional requirement: https://github.com/tahoe-lafs/tahoe-lafs/search?q=numpy&unscoped_q=numpy

@exarkun
Copy link
Contributor Author

exarkun commented Apr 11, 2019

Nothing in misc ends up in output, nor does anything that does end up in output appear to reference numpy. Creating additional outputs for the misc tools might be sensible but I'm not interested in working on that as I've never yet had occasion to use anything from the misc directory.

@dotlambda
Copy link
Member

Alright, I'll merge this. If someone complains, this is still an easy fix.

@GrahamcOfBorg build tahoe-lafs

@dotlambda dotlambda merged commit be8f891 into NixOS:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants