lynxthecat / adblock-lean

Lean and powerful adblocking solution for OpenWrt
https://forum.openwrt.org/t/adblock-lean-set-up-adblock-using-dnsmasq-blocklist/157076
98 stars 9 forks source link

ImmortalWrt 21.02.7 dnsmasq-full error #77

Open hungtoong opened 1 week ago

hungtoong commented 1 week ago

root@ImmortalWrt:~# uclient-fetch https://raw.githubusercontent.com/lynxthecat/a dblock-lean/main/adblock-lean -O /etc/init.d/adblock-lean Downloading 'https://raw.githubusercontent.com/lynxthecat/adblock-lean/main/adblock-lean' Connecting to 185.199.109.133:443 Writing to '/etc/init.d/adblock-lean' /etc/init.d/adblock- 100% |***| 67902 0:00:00 ETA Download completed (67902 bytes) root@ImmortalWrt:~# chmod +x /etc/init.d/adblock-lean root@ImmortalWrt:~# service adblock-lean gen_config # generates default config in /root/adblock-lean/con fig /etc/rc.common: /etc/init.d/adblock-lean: line 990: syntax error: unexpected "(" root@ImmortalWrt:~# uci add_list dhcp.@dnsmasq[0].addnmount='/bin/busybox' && uci commit # Optional/reco mmended. Enables blocklist compression to reduce RAM usage root@ImmortalWrt:~# service adblock-lean enable # this will allow the script to automatically run on boo t /etc/rc.common: /etc/init.d/adblock-lean: line 990: syntax error: unexpected "(" root@ImmortalWrt:~#

lynxthecat commented 1 week ago

ImmortalWrt is not supported. Use OpenWrt.

See: https://github.com/lynxthecat/adblock-lean/issues/73

hungtoong commented 1 week ago

Thank u

friendly-bits commented 6 days ago

@lynxthecat looking again at the code, there is no critical functionality depending on process substitution. We have 7 instances, with 5 of them implementing counting entries and/or bytes, 1 implementing the rogue elements check and 1 involved in the version update. If we really wanted to support Busybox ash without process substitution, we could add a config option which would essentially disable element count checks, size checks and rogue element checks, and change the code for the update command to get rid of process substitution there.

I don't have a strong opinion on this, and I certainly don't care about ImmortalWrt, but this could potentially make the app compatible with older versions of OpenWrt which some people are still running.

lynxthecat commented 6 days ago

Sounds reasonable to me, but would it involve a lot of extra coding?

friendly-bits commented 6 days ago

I'd estimate probably up to 50-70 extra lines of code. Actually even a config option is unnecessary: simply detect ash version and disable the features automatically if it doesn't support process substitution.

friendly-bits commented 6 days ago

One question though: which was the first version of OpenWrt which had the updated dnsmasq? We should compare that to the first version which had ash support for process substitution. If these changes happened with the same version change then it's questionable whether implementing this makes sense.

lynxthecat commented 6 days ago

@Wizballs was the first to identify this:

https://forum.openwrt.org/t/adblock-oisd-22-03-allows-you-to-use-huge-blocklists-with-dnsmasq/136413

https://thekelleys.org.uk/dnsmasq/CHANGELOG

friendly-bits commented 6 days ago

So the first Busybox version to support process substitution was 1.34. OpenWrt 22.03 was the first version which had Busybox version with process substitution support. 22.03 was also the first version with the updated dnsmasq.

So basically OpenWrt versions 22.03 and higher already support all adblock-lean features, and all earlier versions don't support neither process substitution nor blocklist compression nor decreased blocklist memory footprint. Not sure it makes sense to change the code in order to re-add support for versions on which adblock-lean won't be as memory-efficient. Maybe? What do you guys think? @Wizballs @lynxthecat @dave14305 @rickparrish

lynxthecat commented 6 days ago

There are often posts on the OpenWrt forum from variants of OpenWrt and these are typically shutdown pretty quickly on the basis that support is unreasonably being sought from OpenWrt for something other than OpenWrt.

Personally I think that, in keeping with what you were indicating above, it makes sense to take some small measures to include compatibility where it won't hurt performance in OpenWrt, but this only stretches so far, and this, combined with the outdatedness of this ImmortalWrt version and the lack of support for some of the key elements that you have identified makes me think that taking extra steps is not warranted here.

The saying comes to mind that the dog should wag the tail rather than the tail wag the dog. If ImmortalWrt and other variants want to provide OpenWrt functionality then it is up to their developers to ensure the same, and it ought not to be incumbent on OpenWrt to fix issues that arise in respect of variants of OpenWrt.

friendly-bits commented 6 days ago

I agree about ImmortalWrt but the basic issue is the same with older versions of OpenWrt (prior to 22.03). So the question is whether it makes to re-add support for the sake of those older versions. It is possible that adblock-lean would be the best dns-based adblocking solution for them, even despite the limitations. I don't have a strong preference to one side or another.

Wizballs commented 5 days ago

Was it only the 'tee' command that is preventing it running on earlier busybox versions?

@hungtoong could you please try installing package 'coreutils-tee' and try adblock-lean again.

I replaced the built in tee with coreutil-tee on my system, and adblock-lean ran and completed as per usual. So there is a chance this could work for you.

Wizballs commented 5 days ago

earlier versions don't support neither process substitution nor blocklist compression nor decreased blocklist memory footprint

@friendly-bits intermediate compression should still work, just not the final Blocklist compression. Interested to see if hungtoong can simply install coreutils-tee for this to work.

friendly-bits commented 5 days ago

try installing package 'coreutils-tee' and try adblock-lean again

Unfortunately, this is not a limitation of tee. The limitation is in the Busybox ash command interpreter which in Busybox versions prior to 1.34 doesn't understand the syntax >(...) which is called process substitution. This syntax is not compliant to the POSIX standard, so technically ash doesn't actually have to implement it at all, but thankfully they did, which enabled us to make these beautiful long pipes. What @hungtoong could actually try is installing and using the Bash command interpreter to run adblock-lean. They would need to replace the "shebang": #!/bin/sh /etc/rc.common -> #!/bin/bash /etc/rc.common. I think (?) this may work.

But generally I agree with @lynxthecat that we shouldn't make this a support ticket for this specific case, considering it's related to ImmortalWrt rather than OpenWrt. What I'm interested to get from this ticket is a decision on whether it makes or doesn't make sense to add some more code which would disable certain features (element count checks, file size checks, rogue elements check, final compression) when running on OpenWrt versions prior to 22.03 in order to re-add support for these versions.

Wizballs commented 5 days ago

@friendly-bits ok thanks good to know. Potentially could include an optional command to replace the shebang (y/n) if this works.