Windows build broken because SO_BINDTODEVICE is not defined

[alerque]

PR #408 has broken my static Windows build, because SO_BINDTODEVICE is not defined. Any suggestions how to fix it? (Also IFNAMSIZ not defined.)

Originally posted by @rpatters1 in #408 (comment)

[alerque]

This highlights our lack of Windows expertise and testing! @rpatters1 If you can help out with a complete test case that would be good, and if you can figure out how to run a CI job that shows the failure that would be even better. What command are you using to do a static build?

@leso-kn Any ideas here?

[rpatters1]

I have created my own Visual Studio solution and project with the source code imported. They build a static library that I embed directly into my program. As far as I can tell, neither SO_BINDTODEVICE nor IFNAMSIZ are defined anywhere in that source code. My recommendation would be to add (perhaps in options.h or options.c) default values:

#ifndef SO_BINDTODEVICE
#define SO_BINDTODEVICE <some-default>
#endif

#ifndef IFNAMSIZ
#define IFNAMSIZ <some-default>
#endif

This is not specifically a windows issue, but rather an issue with ad-hoc builds that aren't on Apple.

[rpatters1]

P.S. I would submit a PR, but I don't know enough about the feature to submit sensible default values.

[leso-kn]

@alerque Yes, my apologies, I've spotted this with a friend yesterday and we've already fixed it (an #if defined() covering both MSVC and msys/mingw/the likes).

I would prefer to submit it alltogether with a mature CI pipeline for windows, however the critical patch is already present on the branch and can be cloned or cherry-picked if urgent

[rpatters1]

I don't think this is a win-specific problem. It seems like it would also be present on non-Apple Unix/Linux builds. But I'm perfectly able to fall back to the previous commit. That's what I did to move forward.

[alerque]

I have cherry-picked 0bc8c56 straight into the master branch so that the dev version rockspec should work on Windows again. That being said the mechanism being used to avoid platforms seems like it could be some feature detection instead of a platform blacklist. In any case I'll leave this issue open to track any adjustments to the fix plus the CI pipeline in the works there.

[leso-kn]

Alright, very good. I have some time today to get the pipeline working and will try to do so! Having that should hopefully prevent this kind of mistake in the future – again my apologies.

Going over the options of phrasing the #ifdef again, it appears one can actually check for the symbol itself:
Being defined as C macro, it's possible to check for #ifdef SO_BINDTODEVICE instead of platform definitions.

@alerque Considering 0bc8c56 is already present on master, do you prefer to receive an updated commit to replace (amend) the previous one or rather a second commit to go on top of 0bc8c56? I can submit either.

[alerque]

A second one to go on top. Anything pushed to the master branch should be considered immutable except in emergencies. What we need is just a small refactor commit to clean it up based on what is there now.

[leso-kn]

Agreed. Alright then, I've pushed that to the previous branch again: 57e490f, you can cherry-pick it from there or I can open a merge request if preferred.

[alerque]

I'd prefer a PR since we're past the "we broke master" phase and can test and merge at our leisure. Given that I can't test it locally I'd also be fine if you combine it with the Windows CI workflow as a proof of concept as well.

[leso-kn]

That sounds reasonable :) ok then, I'll submit it together with the workflow configuration once that is ready. Thanks for the quick processing!

[leso-kn]

Done! :) See !415