-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
url-bot-rs: init at 0.3.1; nixos/url-bot-rs: init #105315
base: master
Are you sure you want to change the base?
Conversation
I'd totally be down for writing a test, but we'd need to spin up some IRCd. If someone can recommend an IRCd that I can spin up with just a few lines of config, that'd be great. |
Patch to make it build on darwin: diff --git a/pkgs/applications/networking/irc/url-bot-rs/default.nix b/pkgs/applications/networking/irc/url-bot-rs/default.nix
index 97445029dce5..5fe7f631bc2b 100644
--- a/pkgs/applications/networking/irc/url-bot-rs/default.nix
+++ b/pkgs/applications/networking/irc/url-bot-rs/default.nix
@@ -1,9 +1,11 @@
-{ lib
+{ stdenv
, rustPlatform
, fetchFromGitHub
, pkg-config
, openssl
, sqlite
+, libiconv
+, Security
}:
rustPlatform.buildRustPackage rec {
@@ -21,13 +23,13 @@ rustPlatform.buildRustPackage rec {
nativeBuildInputs = [ pkg-config ];
- buildInputs = [ openssl sqlite ];
+ buildInputs = [ openssl sqlite ] ++ stdenv.lib.optionals stdenv.isDarwin [ libiconv Security ];
preBuild = ''
export HOME=$TMPDIR
'';
- meta = with lib; {
+ meta = with stdenv.lib; {
description = "Minimal IRC URL bot in Rust";
homepage = "https://github.com/nuxeh/url-bot-rs";
license = licenses.isc;
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 4df81a9efa63..0a8666e369a7 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -7901,7 +7901,9 @@ in
uriparser = callPackage ../development/libraries/uriparser {};
- url-bot-rs = callPackage ../applications/networking/irc/url-bot-rs {};
+ url-bot-rs = callPackage ../applications/networking/irc/url-bot-rs {
+ inherit (darwin.apple_sdk.frameworks) Security;
+ };
urlscan = callPackage ../applications/misc/urlscan { }; |
This is awesome, thanks for taking the time to package @mweinelt, totally agree about testing, this is something I've been thinking about for a long time, but have no personal experience with nix tests, so wasn't sure about the correct way to do it. For my own testing I tend to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mweinelt exceptional work as always! I left a few optional suggestions that are worth bonus points, if you're interested.
Added |
@mweinelt I made a PR mweinelt#4 against this branch to update the module and the config generation to make it feature complete, allowing a number of things to be configured, but primarily for connecting to multiple networks and enabling other additional features on a per-network basis. Also, tested thoroughly using |
Result of 1 package blacklisted:
1 package built:
|
I marked this as stale due to inactivity. → More info |
Would like to get this merged. I see the benefit of automatic toml generation from expressions - seems to be the modern way, while also feel an ergonomic version as I proposed in the PR i made, is also nice to have, even if the implementation is more complex, it's a conundrum for me. However, one essential feature as I see it is being able to specify multiple networks/connections in the nix. If I can get it to work I might suggest updates to this PR to do this, using the toml generation. |
Btw... it's currently not possible to connect to multiple networks, since the expression will only generate a single configuration, but with the way url-bot-rs is designed, you currently need multiple configuration files to do this. |
Sorry, I was a bit overwhelmed at the time when you put up that huge pull request to my fork and was thinking about how to reconcile that with RFC42 (https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md) and that is still an open question to me. |
Makes total sense, i felt conflicted about the PR myself. Have never seen this RFC, but glad there are some guidelines in this area, otherwise it does seem to be a bit open ended how you implement modules. I think it certainly seems that the expression to toml thing seems like the way to go, I'll see if I can knock up something to allow multiple configs using it. Something like |
From mweinelt#4 (comment)
Is there a good example of a module that supports multiple instances? From what I remember people always just went with NixOS containers to side-step this issue. |
@mweinelt i'm sure i have come across it many times, although the only one i can think of off the top of my head is nginx, for example. However I'm sure all of the cases I have seen are "old school" nix modules. Since url-bot-rs natively supports multiple connections with the same binary, it seems wasteful to spawn multiple copies, also in containers, to me. However, another way to sidestep this would be for url-bot-rs to accept a single config containing multiple networks, in a future release. I'd be quite happy to do that, it seems like a simpler option overall, and it would be better to get this merged sooner rather than later! |
Sounds good to me. |
The 0.3.2 update chokes reproducibly on a test:
|
@mweinelt 0.3.2 hasn't changed, it's just outstanding stuff that hasn't had a release yet. But indeed, have noticed this too, I think it is intermittent. Perhaps something about the port it uses in the test. I have multi configs in nuxeh/url-bot-rs#420 (!) which appears to work, just having a final look over it, and still some tests to sort out, but I think this could be the basis of a new version. |
https://github.com/nuxeh/url-bot-rs/blob/1ccfadcf79b75ec186b7052fb06f028fc38edc3e/src/plugins/vimeo.rs#L119-L124 These two want to bind to the same port, maybe that is the problem? |
So now there's https://github.com/nuxeh/url-bot-rs/releases/tag/v0.4.0-rc.1. Still think a bit more testing is needed which I can't do till tomorrow, but it's a start at least. Wondering if a HashMap TOML serialisation might map better to a Nix expression, rather than a vector, maybe I'll play around with that some more, also. |
https://github.com/nuxeh/url-bot-rs/releases/tag/v0.4.0-rc.2 is a hash map, think this is much better. Also, incase it tickles anyone's fancy I'll work on getting shell completion generation quite soon. |
690c9a8
to
8c9c4c9
Compare
url-bot-rs is a minimal IRC bot that looks up URLs posted on IRC and extracts title and other metadata and relays it back to the IRC channel it was originally mentioned.
@mweinelt I made mweinelt#9, which is the simplest method i could come up with to allow both the existing module behaviour, and multiple network configurations which are available in url-bot-rs since v0.4.0, and by now have been reasonably well tested. Would be interested to hear what you think. I hope it doesn't obfuscate things too much - i'm thinking mainly that the example I've added for the settings option is a multiple network configuration, which could be confusing, if it were possible to have multiple examples, i'd put a single network there too |
I hope we can update the the module, so we can make the config look like That would be an |
Motivation for this change
Add some sorely needed metadata to IRC conversations.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)@nuxeh