zigdig icon indicating copy to clipboard operation
zigdig copied to clipboard

windows support

Open TheOnAndOnlyZenomat opened this issue 11 months ago • 8 comments

this enables windows support for the library, as discussed in #16 .

things this PR does:

  • implement getAddressList for windows
    • i am not too happy about the error handling and would be happy about any pointers
  • fix connectToResolver compile problem under windows
  • get the main zigdig executable to compile under windows
    • here I am not too happy about depending on one specific DNS server, especially not googles
    • I didn't want to introduce a platform specific cli option
    • I didn't want to enumerate the windows registry to find the configured dns server
    • maybe @lun-4 has an idea here for the UX

TheOnAndOnlyZenomat avatar Feb 13 '25 09:02 TheOnAndOnlyZenomat

One problem still persists: the tests wont run on windows, as the refAllDecls also references a function (fetchTrustedAddresses), which will only ever be called in a supported context, but throws a compile error, if it isn't. I don't want to include os specific logic in that file, since it is never used in another context, different from where it works

TheOnAndOnlyZenomat avatar Feb 13 '25 09:02 TheOnAndOnlyZenomat

I didn't want to introduce a platform specific cli option

could we make the CLI have dig-style nameserver override? would apply to all platforms dig @1.1.1.1 <domain> <rtype>

lun-4 avatar Feb 13 '25 14:02 lun-4

the tests wont run on windows [...] which will only ever be called in a supported context

return error.SkipZigTest; doesn't work? :thinking:

lun-4 avatar Feb 13 '25 14:02 lun-4

I didn't want to introduce a platform specific cli option

could we make the CLI have dig-style nameserver override? would apply to all platforms dig @1.1.1.1 <domain> <rtype>

that would be an option. on windows this option then would be required, instead of optional? or do we still want to depend on a specific dns service as a fallback?

TheOnAndOnlyZenomat avatar Feb 14 '25 08:02 TheOnAndOnlyZenomat

the tests wont run on windows [...] which will only ever be called in a supported context

return error.SkipZigTest; doesn't work? 🤔

wouldn't that require for the function to be referenced in test? it is referenced by a refAllDecls which just references all declarations of a module, so they get analyzed, so we can't return an error.SkipZigTest, because it isn't really a test. (At least thats how I understand it).

TheOnAndOnlyZenomat avatar Feb 14 '25 08:02 TheOnAndOnlyZenomat

the new commit fixes the test failure, but I am not so sure about how "nice" this actually is. check the commit msg for a closer explanation

TheOnAndOnlyZenomat avatar Feb 14 '25 08:02 TheOnAndOnlyZenomat

that would be an option. on windows this option then would be required, instead of optional? or do we still want to depend on a specific dns service as a fallback?

I'd say default to 8.8.8.8, we print the address to the user either way so if they want to override they can.

the new commit fixes the test failure, but I am not so sure about how "nice" this actually is. check the commit msg for a closer explanation

how did you test this? any instructions? I'm a bit confused without having the actual compile error traces. could we make it such that we don't refAllDecls on helpers.zig?

lun-4 avatar Feb 16 '25 23:02 lun-4

For testing, I basically checked if it compiles and runs with zig test. You can try to revert that commit, run zig build and zig build test and it should fail, because of the compile error. with the commit everything should run fine. we could remove the refAllDecls on helpers.zig, but then we wouldn't have proper compile/test checks and anymore, as some of those functions might not be referenced. in that case, proper tests for all relevant functions would be necessary. (at least thats how I think it works, based on my not too advanced level of the zig build and test system)

TheOnAndOnlyZenomat avatar Feb 17 '25 07:02 TheOnAndOnlyZenomat

apologies for delay, got a windows system to develop on and understand what was happening, because SkipZigTest; on non-test code looks very weird IMO

regardless, found the following issues:

  • zig build test wasn't actually running the tests properly it seems
  • changed the test entrypoint from main.zig to test.zig and the refAllDecls calls inside such that connectToSystemResolver() isn't analyzed (as it should be, that's the main cause of the issue. i've fixed it in an TheOnAndOnlyZenomat-master branch in this repo, removing the SkipZigTest and keeping the compile error)

if there's no other wanted changes, I can merge that branch back to master and close this PR with it

lun-4 avatar Mar 14 '25 22:03 lun-4

No worries. Thanks for continuing to look into this. Looking over this, the changes look good, id be happy to get this merged an closed. Thanks again for your help with this issue

TheOnAndOnlyZenomat avatar Mar 14 '25 23:03 TheOnAndOnlyZenomat