Skip to content

trafficshaper: avoid recursive dependency - #30074

Merged
BKPepe merged 1 commit into
openwrt:masterfrom
dhrm1k:trafficshaper-fix-nftables-dependency
Aug 3, 2026
Merged

trafficshaper: avoid recursive dependency#30074
BKPepe merged 1 commit into
openwrt:masterfrom
dhrm1k:trafficshaper-fix-nftables-dependency

Conversation

@dhrm1k

@dhrm1k dhrm1k commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #29830 and the discussion after it was merged.

The conditional nftables dependency causes a recursive Kconfig dependency when
it resolves to nftables-nojson.

Split it into nftables and iptables variants instead.

Package Details

Maintainer: Luiz Angelo Daros de Luca luizluca@gmail.com (@luizluca)
Description:

trafficshaper             nftables (default)
trafficshaper-iptables    iptables

Both install the same init script and config. The iptables backend stays
available for older systems, while the normal package does not pull the
iptables userspace packages into nftables installations.


Run Testing Details

  • OpenWrt Target/Subtarget: x86/64
  • OpenWrt Device: generic x86/64 build

I selected both variants as modules and ran make defconfig. The recursive
dependency is gone and both APKs build.

I also checked the generated dependencies. For the firewall backend, the normal
package only pulls nftables. The iptables variant pulls iptables,
ip6tables, and iptables-mod-conntrack-extra.

Also checked with:

git diff --check
busybox ash -n files/trafficshaper.init

Formalities

  • I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

If your PR contains a patch:

Not applicable. This PR does not add or modify an upstream source patch.

@dhrm1k

dhrm1k commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

geoip-shell and miniumnpd follows similar approach

@dhrm1k

dhrm1k commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@luizluca what do you say about this approach?

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit checks

  • c707ba9 "Merge branch 'master' into trafficshaper-fix-nftables-dependency" — this is a merge commit; OpenWrt PRs must not contain merge commits. Rebase onto current master and force-push so the series is just the single 61f2ebc change commit. (Also flagged by the formality bot.)

The functional change itself looks correct: splitting into a default trafficshaper (nftables) variant plus a trafficshaper-iptables variant via Package/trafficshaper/Default + VARIANT/DEFAULT_VARIANT/CONFLICTS matches the established in-tree pattern (e.g. net/geoip-shell and net/miniupnpd), and the DEPENDS split reproduces the original conditional dependencies (nftables vs. iptables/ip6tables/iptables-mod-conntrack-extra). Depending on +nftables directly rather than the PACKAGE_nftables-json||PACKAGE_nftables-nojson conditional resolves the recursive Kconfig dependency. No further issues found in the diff.


Generated by Claude Code

@AndyChiang888

Copy link
Copy Markdown
Contributor

ping @dhrm1k @BKPepe

@BKPepe

BKPepe commented Jul 28, 2026

Copy link
Copy Markdown
Member

There is merge commit, thus formality check is failing. This needs to be resolved first.

@AndyChiang888

Copy link
Copy Markdown
Contributor

@dhrm1k Please fix this PR, thanks.

The conditional nftables dependency causes Kconfig to select
PACKAGE_nftables-nojson from itself:

  symbol PACKAGE_nftables-nojson is selected by PACKAGE_nftables-nojson

Split the firewall dependencies into nftables and iptables variants. Keep
trafficshaper as the default nftables variant and provide
trafficshaper-iptables for older systems.

Fixes: 2e945de ("trafficshaper: add nftables firewall backend")

Signed-off-by: Dharmik Parmar <dharmikparmar2004@yahoo.com>
@luizluca
luizluca force-pushed the trafficshaper-fix-nftables-dependency branch from c707ba9 to eeb3441 Compare July 28, 2026 12:18
@luizluca

Copy link
Copy Markdown
Contributor

@dhrm1k , it was my mistake. I might have clicked the update button (with merge) while scrolling down in a touch screen device. Sorry. I rebased now to get rid of it.

@dhrm1k

dhrm1k commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

should we merge it now?

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 new commit; no new issues found.

The rebase resolved the merge-commit finding from my previous review — the series is now just the single eeb3441, and FormalityCheck / Git & Commits is green. The net/trafficshaper/Makefile diff is byte-identical to what I reviewed at c707ba9, and the commit message matches the change (the Fixes: 2e945de232bd ("trafficshaper: add nftables firewall backend") trailer resolves correctly).

Re-confirmed a few points against in-tree references while re-checking the final state:

  • The Package/trafficshaper/Default + PROVIDES/VARIANT/DEFAULT_VARIANT/CONFLICTS layout matches net/geoip-shell/Makefile:18-42 almost line for line, including the plain-named default variant plus a -iptables variant carrying CONFLICTS.
  • PROVIDES:=trafficshaper in the Default block is inherited by Package/trafficshaper itself, but that self-provide is filtered out by SanitizeProvides / FormatProvides in package-pack.mk:156-158, so it is a no-op rather than the redundant self-provide apk objects to. It is meaningful on trafficshaper-iptables, which is what makes the two mutually exclusive.
  • Sharing one install recipe between the variants is safe because the init script selects its backend at runtime via command -v nft at trafficshaper.init:68, so the split only changes which userspace packages get pulled in.
  • PKG_RELEASE 4 → 5 is correct for a dependency/packaging-only change with no PKG_VERSION bump.

Nothing blocking from my side.


Generated by Claude Code

$(call Package/trafficshaper/Default)
TITLE+= (nftables)
DEPENDS+= +nftables
VARIANT:=nftables

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure VARIANT works here as we don't have a build step. Anyway, it is, at most, innocuous.

@luizluca luizluca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@egc112

egc112 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

This works for me,thanks @dhrm1k

@AndyChiang888

Copy link
Copy Markdown
Contributor

LGTM

@AndyChiang888 AndyChiang888 mentioned this pull request Aug 3, 2026
4 tasks
@httpstorm

httpstorm commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Thank you!

This removes the nftables-nojson related warning I see when I run:

 make menuconfig

 WARNING: Makefile 'package/feeds/video/sdl3/Makefile' has a build dependency on 'libwayland', which does not exist
-tmp/.config-package.in:106766:error: recursive dependency detected!
-tmp/.config-package.in:106766:	symbol PACKAGE_nftables-nojson is selected by PACKAGE_nftables-nojson
-For a resolution refer to Documentation/kbuild/kconfig-language.rst
-subsection "Kconfig recursive dependency limitations"

Any advice or fix for the remaining libwayland warning is welcome.
I think on macOS host, it can be installed using brew install wayland.
I wonder if there is a requirement to have that installed or if it needs to be fixed?
By the way, please don't leak e-mail addresses in the PR description. The GitHub handle alone is enough.

@BKPepe
BKPepe merged commit 8e912e9 into openwrt:master Aug 3, 2026
14 checks passed
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.

7 participants