windows support
this enables windows support for the library, as discussed in #16 .
things this PR does:
- implement
getAddressListfor windows- i am not too happy about the error handling and would be happy about any pointers
- fix
connectToResolvercompile problem under windows - get the main
zigdigexecutable 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
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
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>
the tests wont run on windows [...] which will only ever be called in a supported context
return error.SkipZigTest; doesn't work? :thinking:
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?
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).
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
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?
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)
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 testwasn't actually running the tests properly it seems- changed the test entrypoint from main.zig to test.zig and the
refAllDeclscalls inside such thatconnectToSystemResolver()isn't analyzed (as it should be, that's the main cause of the issue. i've fixed it in anTheOnAndOnlyZenomat-masterbranch 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
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