Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
darkstat: #29343
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
base: master
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
darkstat: #29343
Changes from all commits
802853c1cb1f50d1d728bae618e87e1dec41d4b4d549ae609File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
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.
Why do we need this now? 🤔
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this
( cd $(PKG_BUILD_DIR) && autoreconf -fi )?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this now?
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
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.
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.
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.
@padre-lacroix Any plans to upstream this or letting the upstream know about this issue?
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.
As incoming maintainer I will follow up with upstream regarding this
patch if @padre-lacroix has not already done so.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.
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.
This version-check override is in a location CI won't discover, so it won't take effect. Every
test-version.shin this repo (65 of them, e.g.libs/expat/test-version.sh) sits in the package root, not in atests/subdirectory. The only threetests/subdirs that exist (net/pbr,net/adblock-fast,net/https-dns-proxy) are unrelatedrun_tests.shsuites, a different mechanism. Placed here atnet/darkstat/tests/test-version.sh, the generic version check will still run and fail (darkstat has no--version). Move the file tonet/darkstat/test-version.sh.Generated by Claude Code