Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSVC support #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

SamuelMarks
Copy link

WiP

PS: Any chance for a license change? - I always use (Apache-2.0 OR MIT) though also have a likening to zlib and CC0.

…`source_group`s [src/{daemon,main,options,utils}.c] MSVC support (WiP); [README.md] Use generator independent build command
@@ -42,7 +44,10 @@ target_link_libraries(violet PRIVATE LibJuice::LibJuiceStatic)

install(TARGETS violet RUNTIME DESTINATION bin)

target_compile_options(violet PRIVATE -Wall -Wextra)
target_compile_options(violet PRIVATE -Wall)
Copy link
Owner

@paullouisageneau paullouisageneau Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does -Wall work for MSVC? Is it translated by the CMake generator?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it didn't error at least

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -27,7 +27,12 @@
#include <stdlib.h>
#include <string.h>
#include <time.h>
#ifdef _MSC_VER
#include <processthreadsapi.h>
#define localtime_r(timer, buf) localtime_s(buf, timer)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the return type is also different, I think defining a proper helper function like in libjuice would be better:

static int get_localtime(const time_t *t, struct tm *buf) {
#ifdef _WIN32
        return localtime_s(buf, t) == 0 ? 0 : -1;
#else // POSIX
        return localtime_r(t, buf) != NULL ? 0 : -1;
#endif
}

@paullouisageneau
Copy link
Owner

Thank you, I dropped Windows support for Violet because it was a bit cumbersome and I didn't think there would be a use case for it, even if the underlying library libjuice is compatible.

PS: Any chance for a license change? - I always use (Apache-2.0 OR MIT) though also have a likening to zlib and CC0.

I could change to GPL but I'm not considering a change to a permissive license, see paullouisageneau/libjuice#115 (comment)

Co-authored-by: Paul-Louis Ageneau <[email protected]>
@SamuelMarks
Copy link
Author

BTW: Two platforms I was thinking to port this to next are iOS and Android. For my TVconsole hobby project.

@@ -85,7 +90,11 @@ int main(int argc, char *argv[]) {
goto error;
}

#ifdef _MSC_VER
SuspendThread(/*TODO*/);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still WIP, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. My main question at the moment is whether running a STUN & TURN server on a node will even make a difference, i.e., if this use-case could be facilitated: https://stackoverflow.com/q/68600741

(my thinking is not which is why I've quasi-abandoned my contribution to the various open-source C STUN/TURN implementations)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STUN est basically the standard way to implement NAT hole-punching (in particular in the context of ICE). The STUN server must be outside of the NAT you are trying to traverse (hole-punching always relies on a reachable third node outside the NAT), therefore, deploying a STUN server on one of the two connecting nodes won't make any difference.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's the same conclusion I came to (eventually).

😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants