AdguardTeam / FiltersCompiler

A tool that compiles & validates filters
GNU Lesser General Public License v3.0
52 stars 12 forks source link

Make "!#include" sensitive to "!#if" #84

Closed DandelionSprout closed 2 years ago

DandelionSprout commented 4 years ago

Okay, so this apparently became a big problem for my list, who NO ONE from the AdGuard team told me had been disabled in Filters Registry for two weeks. I wasn't informed about this until I woke up this morning at https://github.com/DandelionSprout/adfilt/issues/63#issuecomment-642910119.

Long story short, from what I was told on Slack a whole 12 days later, the Filters Compiler tried to load an !#include file even if it's been wrapped into !#if !adguard or !#if ext_ublock. This should most probably be fixed, or alternately described in you guys' syntax guide on your websites as that it can't ever be done.

I ended up using a plan B to hopefully re-instate my list, which I could've performed within 30min if anyone here had just communicated the disablement a bit better with me. Next time anything similar happens, please tell me about such fatal building errors.

krystian3w commented 4 years ago

Maybe old version Compiler no have added !adguard uBO name (with negation).

These no generate errors:

DandelionSprout commented 4 years ago

I thank you for the research, but I believe this problem is specific to AdGuard's tool for processing third-party lists that are included in AdGuard.

Alex-302 commented 4 years ago

@DandelionSprout Hi. The filter was disabled because all filters build failed, if one failed. I see you changed include to !#include AntiAdblockEntries.txt. We will check now.

ameshkov commented 4 years ago

@DandelionSprout sorry, I thought @Alex-302 told you about that:(

@tvinzz please be careful, fixing this may require updating https://github.com/AdguardTeam/FiltersDownloader AND then using it here and in the browser extensions (both WebExtensions and Safari one).

ameshkov commented 4 years ago

Long story short, from what I was told on Slack a whole 12 days later, the Filters Compiler tried to load an !#include file even if it's been wrapped into !#if !adguard or !#if ext_ublock.

Well, it should load any #include actually. Same for uBO.

You see, the difference between uBO and AG is that uBO ignores errors when it loads #include, and we fail the whole process. The reasoning is simple - it is better not to deliver the update than to deliver half of the filter list.

ameshkov commented 4 years ago

Now that I understand that, I don't think we should implement any changes to the filters compiler.

It should fail, I don't see anything wrong with this behavior.

Our mistake was not alarming you about this, and I apologize, this was really strange on our part.

DandelionSprout commented 4 years ago

From what I've loosely figured out, uBO does make "!#include" sensitive to "!#if", though I'm not 110% sure if it was intentional on their part.

This explanation will probably be messy, but essentially:

!#if env_mobile
!#include extensionfile.txt
!#endif

...becomes:

!#if env_mobile
! >>>>>>>> (...)/extensionfile.txt
||example.com^
||example.org^
||example.net^
! <<<<<<<< (...)/extensionfile.txt
!#endif

The fact that this was the way uBO made it sensitive, was something I was unaware of until just now (as I had never really thought about the involved mechanics). Had I known that all !#include lists were in fact loaded by both adblockers under all circumstances, I'd have thought of some different approach to all this all along.

ameshkov commented 4 years ago

Ah, wait, it seems I misunderstood the original issue.

I thought we were talking about a naked #include.

Let me keep this issue open then, we'd better check it.

ameshkov commented 4 years ago

@tvinzz

Steps to reproduce

This filter list should be compiled/downloaded without issues:

!#if non_existing_variable
!#include non_existing_file.txt
!#endif

What to do next

If there are issues and we do download the included file, please comment here. It might not be feasible to change this.

DandelionSprout commented 4 years ago

This whole matter became a bit convoluted.

Essentially, I thought that if an !#include file was wrapped into !#if, then it wouldn't be loaded at all. And if it wasn't loaded at all, then a non-existent or prohibited filename would not cause a build fail.

DandelionSprout commented 4 years ago

I also realise now that uBO also tries to load filepaths that in their case were wrapped into !#if !ext_ublock, to my honest surprise. 😶

(Tested by adding https://raw.githubusercontent.com/DandelionSprout/adfilt/master/Sandbox/!%23include-in-!%23if-test-uBO.txt as a filterlist and getting https://raw.githubusercontent.com/DandelionSprout/adfilt/master/Sandbox/JohnMadden.txt?_=10: 404 Not Found as an error in its logger.)

ameshkov commented 4 years ago

Well, it's just easier to implement it that way.