add dnsmasq for NetworkManager shared network #3865
Conversation
📝 WalkthroughWalkthroughThis pull request introduces changes to two build configuration files in the external buildroot repository. In both the Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
98efed8 to
3e8484f
Compare
|
Hm, ok so there is more than it than just Supervisor changes 😢. It should be fairly low-risk to add We certainly should check that there is no init scripts or systemd units deployed. In a quick look the |
there are also home-assistant/cli#533 and home-assistant/home-assistant.io#37140. (i started on frontend yesterday but, even though i've coded for 50 years, i have never done a front end so i'm having to learn some new tricks. at least retirement is not being wasted!) can you provide some parameters around "fit" for
on i should know more about |
There was a problem hiding this comment.
I guess you added only the platforms you were able to test, however, the options should be added to all the board configs pre-emptively, as there's AFAIK no mechanism that enables the functionality depending on the board in other PRs.
Regarding the concerns @agners raised - the binary itself is ~400 KiB on x86 before FS compression - not great, not terrible. Install seems to be sane and install only the binary and harmless stuff:
>>> dnsmasq 2.90 Installing to target
GIT_DIR=. PATH="/build/output_generic_x86_64/host/bin:/build/output_generic_x86_64/host/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" CC="/build/output_generic_x86_64/host/bin/x86_64-buildroot-linux-gnu-gcc" /usr/bin/make -j33 -C /build/output_generic_x86_64/build/dnsmasq-2.90 COPTS="-DNO_TFTP -DHAVE_DBUS" PREFIX=/usr CFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -O2 -g0 -D_FORTIFY_SOURCE=1" DESTDIR=/build/output_generic_x86_64/target LDFLAGS="" LIBS+= install
make[1]: Entering directory '/build/output_generic_x86_64/build/dnsmasq-2.90'
install -d /build/output_generic_x86_64/target/usr/sbin
install -d /build/output_generic_x86_64/target/usr/share/man/man8
install -m 644 man/dnsmasq.8 /build/output_generic_x86_64/target/usr/share/man/man8
install -m 755 src/dnsmasq /build/output_generic_x86_64/target/usr/sbin
make[2]: Entering directory '/build/output_generic_x86_64/build/dnsmasq-2.90/src'
make[2]: 'dnsmasq' is up to date.
make[2]: Leaving directory '/build/output_generic_x86_64/build/dnsmasq-2.90/src'
make[1]: Leaving directory '/build/output_generic_x86_64/build/dnsmasq-2.90'
mkdir -p /build/output_generic_x86_64/target/var/lib/misc/
/usr/bin/install -m 0644 -D /build/output_generic_x86_64/build/dnsmasq-2.90/dbus/dnsmasq.conf /build/output_generic_x86_64/target/etc/dbus-1/system.d/dnsmasq.conf
What I wonder (but would have to be probably tested), would the DNS resolving, mDNS and whatnot work properly also for the clients? We use fairly exotic setup for that, which might be different from what NM expects. I think @agners is more knowledgeable in this area than me 😇
| @@ -126,6 +126,8 @@ BR2_PACKAGE_IPTABLES_NFTABLES=y | |||
| BR2_PACKAGE_IPTABLES_NFTABLES_DEFAULT=y | |||
| BR2_PACKAGE_NETWORK_MANAGER=y | |||
| BR2_PACKAGE_NETWORK_MANAGER_CLI=y | |||
| BR2_PACKAGE_DNSMASQ=y | |||
There was a problem hiding this comment.
I see this was added manually, ideally make ... savedefconfig result of the modified config (e.g. using menuconfig) should be committed to the repo. The correctly formatted defconfig would have the changes around these lines:
diff --git a/buildroot-external/configs/generic_x86_64_defconfig b/buildroot-external/configs/generic_x86_64_defconfig
index f67c710da..49c0380e9 100644
--- a/buildroot-external/configs/generic_x86_64_defconfig
+++ b/buildroot-external/configs/generic_x86_64_defconfig
@@ -119,6 +119,8 @@ BR2_PACKAGE_BLUEZ5_UTILS_PLUGINS_AUDIO=y
BR2_PACKAGE_BLUEZ5_UTILS_PLUGINS_HID=y
BR2_PACKAGE_DHCP=y
BR2_PACKAGE_DHCP_CLIENT=y
+BR2_PACKAGE_DNSMASQ=y
+# BR2_PACKAGE_DNSMASQ_TFTP is not set
BR2_PACKAGE_DROPBEAR=y
# BR2_PACKAGE_IFUPDOWN_SCRIPTS is not set
BR2_PACKAGE_IPROUTE2=yThere was a problem hiding this comment.
I see this was added manually, ideally
make ... savedefconfigresult of the modified config (e.g. usingmenuconfig) should be committed to the repo.
i'll figure out that as i make the other changes.
| @@ -126,6 +126,8 @@ BR2_PACKAGE_IPTABLES_NFTABLES=y | |||
| BR2_PACKAGE_IPTABLES_NFTABLES_DEFAULT=y | |||
| BR2_PACKAGE_NETWORK_MANAGER=y | |||
| BR2_PACKAGE_NETWORK_MANAGER_CLI=y | |||
| BR2_PACKAGE_DNSMASQ=y | |||
| BR2_PACKAGE_DNSMASQ_TFTP=n | |||
There was a problem hiding this comment.
This is not correct, as you can see above, disabling should be done through the # ... is not set stanza.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
correct, that is in the commit message.
i wondered about that. i'll update all the platforms and commit message, too. |
jkt628
force-pushed
the
feat/ipv4-method-shared
branch
from
3e8484f to
5d92e00
Compare
i have connected my phone and tablet to the shared network and they seem to work fine. the only problem i can remember is frigate-nvr could not connect my cameras due to iptables / netfilter configuration, but that was with home-assistant/supervised-installer and easily fixed, i will have to test again with HAOS. |
initial report: not working, 😢 . i see the AP on my desktop and phone but cannot connect. i found this in the journal: but do not see similar on rpi4_64. digging in.... |
required by home-assistant/supervisor#5577
Summary by CodeRabbit