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

nixos/tox-node: init #59368

Merged
merged 2 commits into from Apr 15, 2019

Conversation

@suhr
Copy link
Contributor

commented Apr 12, 2019

Motivation for this change
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 compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Right now it uses the same uid as tox-bootstrapd, this probably should be changed.

@suhr suhr force-pushed the suhr:tox-node branch 2 times, most recently from fdbfe47 to 3569d48 Apr 12, 2019

cfg = config.services.tox-node;
homeDir = "/var/lib/tox-node";

bootstrapNodes = [

This comment has been minimized.

Copy link
@adisbladis

adisbladis Apr 12, 2019

Member

Where is this list coming from? Is it possible to pull it from the tox-node package?

This comment has been minimized.

Copy link
@suhr

suhr Apr 13, 2019

Author Contributor

From here: https://github.com/tox-rs/tox-node/blob/master/dpkg/config.yml

Is it possible to pull it from the tox-node package?

It is easier to add them into the module.

This comment has been minimized.

Copy link
@aanderse

aanderse Apr 14, 2019

Contributor

@adisbladis does this answer satisfy you?

This comment has been minimized.

Copy link
@adisbladis

adisbladis Apr 14, 2019

Member

I'm reluctant to merge this PR as-is. I don't like the additional maintenance burden put on maintainers while updating the package.
A lot of our users are not on NixOS and will possibly miss updating the service.

It's not a lot of extra lines of code to pull this data from the package.

This comment has been minimized.

Copy link
@suhr

suhr Apr 14, 2019

Author Contributor

I'm going to actually maintain this module.

It's not a lot of extra lines of code to pull this data from the package.

It's not very clear how to do this. Would you like to explain how to pull node list from the package?

This comment has been minimized.

Copy link
@joachifm

joachifm Apr 14, 2019

Contributor

Another possibility is to reconstitute the config out-of-store at runtime

This comment has been minimized.

Copy link
@suhr

suhr Apr 14, 2019

Author Contributor

but does it make sense to have this as an option so the user can override

I believe so.

This comment has been minimized.

Copy link
@adisbladis

adisbladis Apr 14, 2019

Member

I don't think we would need IFD at all.
I'll hack a POC of what I have in mind :)

This comment has been minimized.

Copy link
@adisbladis

adisbladis Apr 14, 2019

Member

Check out my changes in my nixpkgs fork: https://github.com/adisbladis/nixpkgs/tree/tox-node (adisbladis@984461c).

I noticed that the dpkg directory is not in tox-node-0.0.7 yet so I had to resort to using fetchurl on this file separately for now.

Feel free to cherry-pick (and optionally squash) this change if you are satisfied with it.

This comment has been minimized.

Copy link
@suhr

suhr Apr 15, 2019

Author Contributor

Feel free to cherry-pick (and optionally squash) this change if you are satisfied with it.

Done.

@suhr suhr force-pushed the suhr:tox-node branch 2 times, most recently from c931726 to 967a1cd Apr 13, 2019

@suhr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

r?

@aanderse

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

r?

You'll have to elaborate on that.

@suhr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Github says “changes required” even though I've applied necessary changes.

@aanderse
Copy link
Contributor

left a comment

In general LGTM

I'd like to hear back from @adisbladis on the bootstrapNodes first, though.
Bonus points if anyone else who uses tox-node could test this as well.

@adisbladis

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Aside from my notes on hard-coded bootstrap nodes this PR looks fine.
A changelog entry for 19.09 would be great too!

suhr and others added some commits Apr 12, 2019

nixos/tox-node: Dont hardcode bootstrap nodes
Get these from upstream tox-node package instead.
This is likely to cause less maintenance overhead over time and
following upstream bootstrap node changes is automated.

@adisbladis adisbladis force-pushed the suhr:tox-node branch from 63518f4 to 454aa43 Apr 15, 2019

@adisbladis adisbladis merged commit 4b4caa9 into NixOS:master Apr 15, 2019

11 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@adisbladis

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Looks great now! Thanks for your contribution :)

@suhr suhr deleted the suhr:tox-node branch Apr 15, 2019

@samueldr

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

This PR broke tarball build due to the missing descriptions on the module options.

(Sorry if the tone seems a bit rough, just stating what I found out.)


Adding more details, in case it confuses people:

The tarball build is more strict and will abort the build when any warning is produced. I do not know if there are easier methods to catch these warnings other than doing the tarball build.

@joachifm

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

The grahamcofborg-eval-nixpkgs-tarball was reported as successful, is that the same thing? If so, why the discrepancy in outcome?

@samueldr

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@joachifm since it happens at build time no, ofborg doesn't build untrusted inputs automatically, only evals untrusted inputs automatically.

@adisbladis

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Whoops.. Descriptions added in 9a176d6

@samueldr

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Thanks @adisbladis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.