darkstat:#29343
Conversation
3424261 to
81c6b78
Compare
|
darkstat does not implement a --version flag and cannot report its |
|
You can add #!/bin/sh
exit 0Or do this check per sub-package if it only applies to some. |
| PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.bz2 | ||
| PKG_SOURCE_URL:=http://unix4lyfe.org/darkstat | ||
| PKG_HASH:=aeaf909585f7f43dc032a75328fdb62114e58405b06a92a13c0d3653236dedd7 | ||
| PKG_SOURCE:=$(PKG_VERSION).tar.gz |
There was a problem hiding this comment.
This is wrong. There should be PKG_NAME and PKG_VERSION as it was previously.
| PKG_SOURCE_SUBDIR:=$(PKG_NAME)-$(PKG_VERSION) | ||
| PKG_SOURCE_URL:=https://github.com/emikulic/darkstat/archive/refs/tags/ | ||
| PKG_HASH:=5c8e66d4c478b6d7e58f4c842823a09125509bf6851017ff70e32b32ce95b01b | ||
| PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_NAME)-$(PKG_VERSION) |
There was a problem hiding this comment.
Why this was added? This is already default.
| define Build/Configure | ||
| ( cd $(PKG_BUILD_DIR) && autoreconf -fi ) | ||
| $(call Build/Configure/Default) | ||
| endef |
There was a problem hiding this comment.
Thank you for the review. You are correct on all three points. The
PKG_SOURCE_SUBDIR, PKG_BUILD_DIR, and Build/Configure additions were workarounds for the GitHub raw tag tarball format which lacks a pre-generated configure script. Using the '?' suffix on PKG_SOURCE_URL causes the build system to treat it as a properly named release tarball, making all three workarounds unnecessary. They have been removed.
There was a problem hiding this comment.
Why do we need this ( cd $(PKG_BUILD_DIR) && autoreconf -fi )?
There was a problem hiding this comment.
My apologies. I did comment on this aspect of your original comment (#29343 (comment)) without context. I re-post it verbatim:
@BKPepe
The ? suffix approach was investigated but darkstat's GitHub releases only provide raw source snapshots without a pre-generated configure script. The autoreconf step is therefore necessary. unix4lyfe.org is no longer hosting releases and no alternative source for a proper release tarball has been found. Requesting guidance on the preferred approach for packages where upstream only provides raw source snapshots via GitHub tags.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the OpenWrt darkstat package to the latest upstream release and aligns packaging/runtime behavior with upstream capabilities and OpenWrt UCI options.
Changes:
- Bump
darkstatto 3.0.722 and switch source retrieval to GitHub. - Ensure additional UCI options (
hosts_keep,ports_max) are passed to the daemon and adjust runtime directory ownership. - Remove the now-upstreamed local-interface patch and refresh build steps to regenerate bundled assets.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| net/darkstat/patches/101-allow-multiple-local-interfaces.patch | Removes a patch that is now incorporated upstream. |
| net/darkstat/patches/100-do-not-use-NI_IDN.patch | Refreshes patch offsets for the updated upstream source. |
| net/darkstat/files/darkstat.init | Passes additional UCI options and adjusts runtime directory ownership. |
| net/darkstat/files/darkstat.config | Enables export/import/daylog options in the default config. |
| net/darkstat/Makefile | Bumps version/source and adds autoreconf + header generation steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| config_get hosts_max $cfg hosts_max | ||
| CONFIGSTR="$CONFIGSTR${hosts_max:+--hosts-max "$hosts_max"} " | ||
| config_get hosts_keep $cfg hosts_keep | ||
| CONFIGSTR="$CONFIGSTR${hosts_keep:+--hosts-keep "$hosts_keep"} " | ||
| config_get ports_max $cfg ports_max | ||
| CONFIGSTR="$CONFIGSTR${ports_max:+--ports-max "$ports_max"} " | ||
| config_get ports_keep $cfg ports_keep | ||
| CONFIGSTR="$CONFIGSTR${ports_keep:+--ports-keep "$ports_keep"} " |
There was a problem hiding this comment.
The quoted argument pattern in CONFIGSTR is consistent with the existing script style throughout build_config_string(). The four added lines follow the same convention as hosts_max, highest_port, and other existing options. Changing this pattern would require refactoring the entire function which is beyond the scope of this PR.
| export_bool verbose $cfg "--verbose" | ||
| # We need the --no-daemon parameter as with PROCD the process has to run in the background | ||
| CONFIGSTR="$CONFIGSTR--no-daemon " | ||
| CONFIGSTR="$CONFIGSTR--no-daemon " # run process in background |
| set_config_string(){ | ||
| mkdir -p $RUN_D | ||
| chown $USER:$GROUP $RUN_D | ||
| . /lib/functions/network.sh |
| option export_file 'darkstat_export.log' | ||
| option import_file 'darkstat_export.log' | ||
| option daylog_file 'darkstat_daylog.log' |
There was a problem hiding this comment.
The default RUN_D points to /var/darkstat which is on tmpfs (volatile RAM). Export, import and daylog files written there have no flash wear impact. A user would have to explicitly reconfigure RUN_D to a persistent path to affect flash storage, which is a deliberate choice. Leaving the options uncommented documents the available functionality while maintaining safe defaults.
| PKG_RELEASE:=7 | ||
|
|
||
| PKG_MAINTAINER:=Jean-Michel Lacroix <lacroix@lepine-lacroix.info> | ||
| PKG_MAINTAINER:=Paul Doran <RuralRoots@gmail.com> |
| define Build/Compile | ||
| $(HOSTCC) $(PKG_BUILD_DIR)/static/c-ify.c \ | ||
| -o $(PKG_BUILD_DIR)/c-ify | ||
| -o $(PKG_BUILD_DIR)/c-ify | ||
| $(HOSTCC) $(PKG_BUILD_DIR)/static/hex-ify.c \ | ||
| -o $(PKG_BUILD_DIR)/hex-ify |
|
The ? suffix approach was investigated but darkstat's GitHub releases only provide raw source snapshots without a pre-generated configure script. The autoreconf step is therefore necessary. unix4lyfe.org is no longer hosting releases and no alternative source for a proper release tarball has been found. Requesting guidance on the preferred approach for packages where upstream only provides raw source snapshots via GitHub tags. |
d6ee68b to
4ea96ad
Compare
| /etc/config/darkstat | ||
| /etc/config/darkstat |
There was a problem hiding this comment.
Are you sure that this indeed works? Why do you do it?
There was a problem hiding this comment.
Changed from spaces to tab for consistency. The conffiles entry works correctly — confirmed by successful build and installation on OpenWrt One r34412+5-35747c66e1. Also, /rom/etc/config/darkstat is populated correctly
There was a problem hiding this comment.
Nah, obviously this is not working. That is why I asked. How did you test this section?
There was a problem hiding this comment.
I test by syncing my Darkstat branch to my local buildroot via
src-link. Build confirmed on r34654+5-a302626ef4:
darkstat 3.0.722 (using libpcap version 1.10.6 (64-bit time_t, with TPACKET_V3))
However you are correct to question it — I cannot confirm the indented conffiles entry works correctly for backup/restore purposes and it was an erroneous change. It has been reverted to the original unindented form.
cb9fbe5 to
9b5ecae
Compare
f5f5030 to
1fb9863
Compare
|
Friendly ping @BKPepe, @GeorgeSapkin — all review feedback has been addressed and all CI checks are passing. Happy to make any further changes if needed. |
| $(HOSTCC) $(HOST_CFLAGS) $(PKG_BUILD_DIR)/static/c-ify.c \ | ||
| -o $(PKG_BUILD_DIR)/c-ify $(HOST_LDFLAGS) | ||
| $(HOSTCC) $(HOST_CFLAGS) $(PKG_BUILD_DIR)/static/hex-ify.c \ | ||
| -o $(PKG_BUILD_DIR)/hex-ify $(HOST_LDFLAGS) | ||
| ( cd $(PKG_BUILD_DIR) && \ | ||
| ./c-ify style_css <static/style.css >stylecss.h && \ | ||
| ./c-ify graph_js <static/graph.js >graphjs.h && \ | ||
| ./hex-ify favicon_png <static/favicon.png >favicon.h ) |
There was a problem hiding this comment.
This Build/Compile block is necessary because darkstat 3.0.721
introduced hex-ify to replace xxd, reducing build dependencies. In a cross-compilation environment the host cannot execute target binaries, so c-ify and hex-ify must be compiled natively using $(HOSTCC) and run on the host before the cross-compiler processes http.c. HOST_CFLAGS and HOST_LDFLAGS were added per CoPilot suggestion to respect host toolchain
hardening settings. If this is not required or follows a different convention in the OpenWrt tree, I am happy to revert it.
| @@ -1,6 +1,11 @@ | |||
| From: Jean-Michel Lacroix <lacroix@lepine-lacroix.info> | |||
There was a problem hiding this comment.
@padre-lacroix Any plans to upstream this or letting the upstream know about this issue?
There was a problem hiding this comment.
As incoming maintainer I will follow up with upstream regarding this
patch if @padre-lacroix has not already done so.
There was a problem hiding this comment.
As incoming maintainer I will take responsibility for this patch.
The last change to dns.c upstream was approximately 10 years ago and NI_IDN appears to be consistently defined, making upstreaming unlikely to be accepted. The patch will remain as necessary maintenance.
|
Please just remove me as the maintainer of this package as I am no longe working to maintain any OpenWRT package due to retirement! |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 7 new commits; each commit message matches its diff, so no commit-check issues. One inline issue (test-version.sh location) and one nit noted below.
Generated by Claude Code
| @@ -0,0 +1,3 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
This version-check override is in a location CI won't discover, so it won't take effect. Every test-version.sh in this repo (65 of them, e.g. libs/expat/test-version.sh) sits in the package root, not in a tests/ subdirectory. The only three tests/ subdirs that exist (net/pbr, net/adblock-fast, net/https-dns-proxy) are unrelated run_tests.sh suites, a different mechanism. Placed here at net/darkstat/tests/test-version.sh, the generic version check will still run and fail (darkstat has no --version). Move the file to net/darkstat/test-version.sh.
Generated by Claude Code
| define Package/darkstat/description | ||
| darkstat is a packet sniffer that runs as a background process on a cable/DSL | ||
| router, gathers all sorts of statistics about network usage, and serves them | ||
| darkstat is a packet sniffer that runs as a background process on an OpenWrt |
There was a problem hiding this comment.
nit: trailing whitespace at end of this line (after "OpenWrt").
| darkstat is a packet sniffer that runs as a background process on an OpenWrt | |
| darkstat is a packet sniffer that runs as a background process on an OpenWrt |
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the changes since my last review (test-version.sh moved to the package root, and the Makefile trailing-whitespace nit removed) — both correctly address my prior comments. No new issues found.
Generated by Claude Code
Adjust out of box runtime options as default behaviour Signed-off-by: Paul Doran <RuralRoots@gmail.com>
These four lines were missing since the package was added to OpenWrt, causing hosts_keep and ports_max UCI options to be silently ignored and never passed to the darkstat daemon. Signed-off-by: Paul Doran <RuralRoots@gmail.com>
The chroot directory was created as root:root but darkstat drops privileges to nobody:nogroup after chroot, preventing writes to export, import and daylog files. Add chown to set correct ownership before service start Signed-off-by: Paul Doran <RuralRoots@gmail.com>
Bump to current upstream release 3.0.722 sourced from GitHub as the original unix4lyfe.org hosting is no longer available. Update PKG_SOURCE_URL and PKG_SOURCE accordingly. Replace maintainer with Paul Doran <RuralRoots@gmail.com> as maintainer per explicit invitation from Jean-Michel Lacroix in issue openwrt#20979. Fixes: openwrt#20979 Fixes: openwrt#26107 Signed-off-by: Paul Doran <RuralRoots@gmail.com>
Functionality incorporated natively in upstream 3.0.722. Verified by building without the patch and running on OpenWrt One r34263+5-164428d2d6 with darkstat operational. Signed-off-by: Paul Doran <RuralRoots@gmail.com>
Refresh patch to apply cleanly against 3.0.722 with no offsets. Signed-off-by: Paul Doran <RuralRoots@gmail.com>
darkstat does not implement a --version flag and cannot report its version string to stdout. Skip the generic version check accordingly. Signed-off-by: Paul Doran <RuralRoots@gmail.com>
Formality Check: FailedWe completed the verification flow. Please review the formatting overview logs below. 🛑 CRITICAL ERRORS
Tip Do not close this pull request to make corrections. Instead, modify your existing commits (e.g. Something broken? Consider reporting an issue. |
📦 Package Details
Maintainer: @ruralroots
(You can find this by checking the history of the package
Makefile.)Description:
Bumps darkstat to current upstream release 3.0.722. The original
unix4lyfe.org hosting is no longer available so PKG_SOURCE_URL
has been updated to GitHub.
Fixes missing hosts_keep and ports_max UCI options that were never
passed to the darkstat daemon since the package was added to OpenWrt.
Fixes chroot directory permissions that prevented export, import and
daylog files from being written.
Removes 101-allow-multiple-local-interfaces.patch as this is now
incorporated natively in upstream 3.0.722.
Add Paul Doran as maintainer per invitation in #20979.
Fixes: #20979
Fixes: #26107
🧪 Run Testing Details
✅ Formalities
If your PR contains a patch:
git am(e.g., subject line, commit description, etc.)
We must try to upstream patches to reduce maintenance burden.