holesail: init at 1.10.1#432474
Conversation
3a79a4a to
0c9c6e4
Compare
There was a problem hiding this comment.
| rev = "refs/tags/${version}"; | |
| tag = finalAttrs.version; |
There was a problem hiding this comment.
Nit: Use finalAttrs is a good practice as it's override friendly
There was a problem hiding this comment.
| repo = pname; | |
| repo = "holesail"; |
We should not use pname as that is wrong when changed. Please use the literal string.
There was a problem hiding this comment.
Nit: Using with lib; in a large attrset is generally discouraged since it's ambiguity and leads to potential name collisions
There was a problem hiding this comment.
| description = "Peer-to-peer network tunneling and reverse proxy software."; | |
| description = "Peer-to-peer network tunneling and reverse proxy software"; |
https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#meta-attributes
There was a problem hiding this comment.
| ]; | |
| ]; | |
| mainProgram = "holesail"; |
|
I updated the commit following your recommendations. |
|
I got a different hash. Did something not get updated? specified: sha256-eANDS4Avu6or+1XXRmM+rceZzjKchhoc5nbv1grpvwE= |
|
I suppose this happened because now it doesn't fetch the zip file from the releases. I changed the hash as well as the npmdeps hash |
|
There was a problem hiding this comment.
LGTM. I noticed this package should also work on Darwin. I’m fine with either adding Darwin support or leaving it as is.
|
The only reason that i didn't add darwin in the available platforms, is that i haven't checked if this build process works well on darwin. I can't check it myself because i don't own a Mac. I was thinking that we could add another maintainer at some point if anyone tries it on Mac. Also, i will notify the owner of holesail repo and ask them if they want to be added as maintainers |
|
In the future i am planning to start a PR with a NixosModule for holesail. I have already built it, but it needs some polishing. |
|
Friendly ping - |
|
Friendly ping :) |
|
Hey @jjacke13, thanks for working on this :). If you're still interested, would you be willing to bump the version? (Also noting there was a license change.) If you're willing, I'd also be happy to be a co-maintainer. |
Yes! Thanks for your interest! I will do it in the next couple of days :) |
|
Bumped to 2.4.1 and updated the license to AGPL-3.0. @GraysonTinker I'd be happy to add you as co-maintainer! You'll need to add yourself to |
Thanks! I'm already listed; I think you'll need to rebase your branch. |
|
Added you as co-maintainer! |
GraysonTinker
left a comment
There was a problem hiding this comment.
Seems like the checks are now failing because of a new requirement; should be fairly easy to resolve.
| npmPackFlags = [ "--ignore-scripts" ]; | ||
|
|
||
| dontNpmBuild = true; | ||
|
|
There was a problem hiding this comment.
| __structuredAttrs = true; |
There was a problem hiding this comment.
buildNpmPackage currently doesn't support __structuredAttrs. The issue is that npmDeps and npmBuildScript are set as top-level derivation attributes (here), so with structured attrs they end up in NIX_ATTRS_SH_FILE as bash variables but aren't exported — meaning the prefetch-npm-deps Rust tool can't see them via env::var_os() and panics.
The fix would be in build-npm-package/default.nix:
- Add
excludeDrvArgNames = [ "npmDeps" "npmBuildScript" ];to avoid conflicts - Move
inherit npmDeps npmBuildScript;into theenvattrset
I have this working locally — happy to include it as a separate commit in this PR, or as a prerequisite PR if preferred. Though I might be missing something here — let me know if you're aware of a simpler way to handle this!
There was a problem hiding this comment.
Oh, sorry for missing that. After looking around a bit I think a fix has already been merged to staging in #487974, and will be merged to master in #507470. I'm not sure what the timeline for that PR to get merged would be but I suspect it will be in the near future. (If I cherry-pick that commit it works.) For now I think there's nothing to do but wait -- thanks for sticking through this.
|
|
||
| npmPackFlags = [ "--ignore-scripts" ]; | ||
|
|
||
| dontNpmBuild = true; |
There was a problem hiding this comment.
Just noting that it seems like there could be a possibility of building to a native binary with the upstream scripts now. But that doesn't have to be done right now, maybe I'll take a crack at it in a follow-up.
There was a problem hiding this comment.
i 've been working on a C++ implementation of the underlying protocol (HyperDHT). It is working pretty well and in the short future we can have holesail written in any language we like ;) so single binary will be trivial ;)
|
@jjacke13 Just in case you didn't know, those changes are now merged and this can be updated. |
|
Rebased on latest master and added |
|
GraysonTinker
left a comment
There was a problem hiding this comment.
Tested and seems like it's working good.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.