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

getdns: cleanup the config and add libevent2 support #17450

Merged
merged 3 commits into from Jan 3, 2022

Conversation

rsalvaterra
Copy link
Member

A couple of commits enabling $subj. First we make the config options a bit less cryptic, then we enable support for compiling with a libevent2-based event loop. I've been carrying these patches in my tree for over a year, building and running on several architectures (x86-64, ramips/mt7621, ath79/generic, mvebu/cortexa9) without any issues whatsoever.

Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>

Maintainer: @jonathanunderwood

@rsalvaterra rsalvaterra changed the title stubby: cleanup the config and add libevent2 support getdns: cleanup the config and add libevent2 support Dec 28, 2021
@jonathanunderwood
Copy link
Contributor

jonathanunderwood commented Dec 28, 2021

LGTM.

Aside: Ideally the same approach should be adopted for the stubby package as well, which, at the moment, just hard removes libunbound and libidn2 dependencies rather than making them options.

@rsalvaterra
Copy link
Member Author

Oh, this is confusing… Are you sure stubby itself directly links to libunbound and libidn2? As far as I can tell, looking through stubby's CMakeLists.txt, I only see find_package looking for libsystemd, libyaml and getdns.

@jonathanunderwood
Copy link
Contributor

Oh, this is confusing… Are you sure stubby itself directly links to libunbound and libidn2? As far as I can tell, looking through stubby's CMakeLists.txt, I only see find_package looking for libsystemd, libyaml and getdns.

I am actually not 100% sure, as I haven't maintained stubby or getdns for many months. another contributer added this to the stubby Makefile build prepartion step though:

rm $(PKG_BUILD_DIR)/cmake/modules/FindLibidn2.cmake $(PKG_BUILD_DIR)/cmake/modules/FindLibunbound.cmake

To prevent libidn2 and libunbound being pulled in as dependencies. I was suggesting that it might be better to offer those as confiuration options. But, perhaps that makes no sense - I am not sure.

These cmake modules are actually never referenced. Stubby itself doesn't link to
libidn or libunbound, only getdns does. They're most likely leftovers from when
stubby was split from getdns to its own repository.

Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
@rsalvaterra
Copy link
Member Author

Adding @neheb to the party. 😉

So, after digging for a while, it seems that those CMake modules are just a reminiscence from when stubby was split from getdns to its own repository. I added a first commit to the pull request partially reverting those changes. It would also probably be a good idea to send a pull request upstream removing those modules, but that's another story.

@neheb neheb merged commit 978e226 into openwrt:master Jan 3, 2022
@rsalvaterra rsalvaterra deleted the stubby branch January 3, 2022 10:44
@anomeome
Copy link

anomeome commented Jan 3, 2022

Broken for me, starting from a rm -rf bin tmp build_dir

Package stubby is missing dependencies for the following libraries:
libunbound.so.8
make[3]: *** [Makefile:76: /home/kc/wrtpac/source/bin/targets/mvebu/cortexa9/packages/stubby_0.4.0-5_arm_cortex-a9_vfpv3.ipk] Error 1
make[3]: Leaving directory '/home/kc/wrtpac/source/feeds/packages/net/stubby'
time: package/feeds/packages/stubby/compile#0.17#0.02#0.17
    ERROR: package/feeds/packages/stubby failed to build.
make[2]: *** [package/Makefile:116: package/feeds/packages/stubby/compile] Error 1

config is unbound built in to image, and stubby as an installable module

@neheb
Copy link
Contributor

neheb commented Jan 3, 2022

It’s worse than that:

Package stubby is missing dependencies for the following libraries:
libidn2.so.0
libunbound.so.8

@rsalvaterra
Copy link
Member Author

It’s worse than that:

Package stubby is missing dependencies for the following libraries: libidn2.so.0 libunbound.so.8

How odd, I haven't hit that! Let me test here…

@rsalvaterra
Copy link
Member Author

Alright, I think I'm seeing a potential issue. I wonder why I haven't hit it at all…? Let me cook a patch…

@dhewg
Copy link
Contributor

dhewg commented Jan 4, 2022

While adding patches... It's not clear why this was added. Why/when would one want libevent2 support? What does one gain?

@rsalvaterra
Copy link
Member Author

While adding patches... It's not clear why this was added. Why/when would one want libevent2 support? What does one gain?

That's a wonderful question. Ideally, performance, with many concurrent requests. In reality, absolutely nothing: I spent the last hours digging through the getdns source and it seems like it just uses poll() or select() (defaults to the former) internally. The libevent2/libev/libuv support are just extensions, and applications need to specifically programmed use them, which isn't the case for stubby, at least. I assumed they'd be used automatically if enabled, but alas. My mistake, I shall be more careful in the future. The status quo, however, is kept, as libevent2 support is disabled by default. To be honest, I only added this because I was already stuck with the library for other reasons (it's required by tor) and thought "well, if I need to have it, I might as well use it".

@dhewg
Copy link
Contributor

dhewg commented Jan 5, 2022

Alright, even that already helps, thanks for digging in!
Maybe it's useful in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants