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

nodogsplash: update to 5.0.1 #997

Merged
merged 1 commit into from
Jul 23, 2023
Merged

nodogsplash: update to 5.0.1 #997

merged 1 commit into from
Jul 23, 2023

Conversation

mwarning
Copy link
Contributor

Maintainer: me
Compile tested: Nexx wt3020 ramips/mt7620, OpenWrt master
Run tested: same as compile target

Description:
Tested Internet access via accept button on the splash site.

@mwarning
Copy link
Contributor Author

@lynxis
@bluewavenet

fyi

@bluewavenet
Copy link
Contributor

@mwarning
The fix looks good, but you should make iptables-nft a dependency, rather than just "iptables" which will result in iptables-zz-legacy being installed which in turn clashes with firewall4 and can result in, at best, weird issues and at worst, failure of NoDog.

@mwarning
Copy link
Contributor Author

@bluewavenet good point, but then +iptables-nft, +iptables-mod-nat-extra and others?

@bluewavenet
Copy link
Contributor

@mwarning

+iptables-nft, +iptables-mod-nat-extra and others?

Yes, leave the iptables-mod dependencies unchanged as they are all compatible with iptables-nft.

ie:

DEPENDS:=+libpthread +libmicrohttpd-no-ssl +iptables-nft \
           +iptables-mod-nat-extra +iptables-mod-ipopt \
           +iptables-mod-conntrack-extra

@bluewavenet
Copy link
Contributor

@mwarning
Oh, sorry, I forgot, you should also have:
CONFLICTS:=opennds

The truly ancient nodogsplash2 conflict can be safely dropped I think ;-)

@mwarning
Copy link
Contributor Author

I we list opennds as conflicting software, then we could list all other captive portals as well...

@bluewavenet
Copy link
Contributor

@mwarning

then we could list all other captive portals as well...

There is some argument to that, eg CPD port 80 redirect is common to all, but I don't think we need to go that far.

But more fundamentally, both NoDogSplash and openNDS have many commonalities such as packet marking that they cannot both be running on the same device at the same time.

Also, since release v10, openNDS has been migrated 100% to native nftables and will be a higher priority in the nft ruleset than any legacy iptables setup, meaning openNDS will always do the port 80 capture regardless of which of NoDog or openNDS was installed first. But then the legacy iptables tables/chains of NoDog will still be in the ruleset.

"Havoc will ensue" :-D

To put it simply, replacing the long defunct nodogsplash2 with the current opennds in CONFLICTS is the sensible way forward, for the same reasons that nodogsplash2 was in there in the first place.

Note: openNDS has nodogsplash in CONFLICTS already.

@mwarning
Copy link
Contributor Author

ok, no problem. I do not have a strong opinion here. :)

@bluewavenet
Copy link
Contributor

@mwarning
Sorry, one last thing, (no more, I promise) the description should be updated to remove the API sentence that should have been deleted when FAS was removed.
ie

define Package/nodogsplash/description
  Nodogsplash is a Captive Portal that offers a simple way to
  provide restricted access to the Internet by showing a splash
  page to the user before Internet access is granted.
endef

Signed-off-by: Moritz Warning <[email protected]>
@bluewavenet
Copy link
Contributor

@mwarning
I'd say it's good to go now ;-)

@mwarning mwarning merged commit 91e0790 into openwrt:master Jul 23, 2023
11 checks passed
@hnyman
Copy link
Contributor

hnyman commented Jul 27, 2023

Now there is circular reference:

tmp/.config-package.in:91534: symbol PACKAGE_nodogsplash depends on PACKAGE_opennds
tmp/.config-package.in:91553: symbol PACKAGE_opennds depends on PACKAGE_nodogsplash

Mutual CONFLICTS is unnecessary and causes this error.

@mwarning
Copy link
Contributor Author

@hnyman can you tell us why this is a problem and how to reproduce it? So we can fix this quickly.

@hnyman
Copy link
Contributor

hnyman commented Jul 27, 2023

You can reproduce it simply by doing "make defconfig" or "make menuconfig" on an up-to-date build env...

perus@ub2304:/Openwrt/e8450$ make defconfig
tmp/.config-package.in:91534:error: recursive dependency detected!
tmp/.config-package.in:91534:	symbol PACKAGE_nodogsplash depends on PACKAGE_opennds
tmp/.config-package.in:91553:	symbol PACKAGE_opennds depends on PACKAGE_nodogsplash
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"

#
# No change to .config
#

Naturally also buildbots see the problem:

E.g.
https://buildbot.staging.openwrt.org/master/packages/#/builders/8/builds/48/steps/24/logs/stdio

Config-build.in:14067:warning: defaults for choice values not supported
tmp/.config-package.in:81252:error: recursive dependency detected!
tmp/.config-package.in:81252:	symbol PACKAGE_nodogsplash depends on PACKAGE_opennds
tmp/.config-package.in:81271:	symbol PACKAGE_opennds depends on PACKAGE_nodogsplash
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"
#
# configuration written to .config

There should be no recursive dependencies, as it can confuse the dependency evaluation logic.

@mwarning
Copy link
Contributor Author

Interesting is that it does not seem to prevent me from building packages/images.

@bluewavenet
Copy link
Contributor

@hnyman @mwarning

There should be no recursive dependencies

Of course, but why does CONFLICTS make a dependency? ( it also tells opkg not to install if the conflicting package is already installed which is what we wanted to achieve.)

@hnyman I cannot find CONFLICTS in any documentation, is it mentioned anywhere? Is there any other way of informing opkg of a "conflict"?

Well the answer is to take the CONFLICTS out of the makefiles of both nodogsplash and opennds.

I'll do it on opennds. I guess adding some code into opennds to look for nodogsplash is one way ;-)

@hnyman
Copy link
Contributor

hnyman commented Jul 27, 2023

"CONFLICTS" gets handled as a negative dependency in the kernel config language.

The best underlying documentation is probably at the site given in the (kernel config) error message:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild

the answer is to take the CONFLICTS out of the makefiles of both nodogsplash and opennds.

Just from one of them, so that the recursive/circular aspect gets removed.

@bluewavenet
Copy link
Contributor

@hnyman

Just from one of them, so that the recursive/circular aspect gets removed.

Understood. Although I will remove it from the opennds makefile as well and instead add a startup check to the opennds code - that will allow both to be installed and a simple config change to switch between the two. The reasoning behind it is that many academic projects start with nodogsplash and move to opennds for api support and currently I end up repeatedly explaining what is wrong!

@bluewavenet
Copy link
Contributor

@mwarning @hnyman
As both NodogSplash and openNDS both have ndsctl as a common resource, it turns out that opkg is clever enough to abort the install of one if the other is present.
This means the presence of CONFLICTS in the makefile has most likely never been necessary, even for the ancient nodogsplash2.
In the case of installing on a non-openwrt Linux, it is a different story, so I am still going to add the startup check in opennds.

@BKPepe BKPepe mentioned this pull request Jul 30, 2023
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.

3 participants