cisco / libsrtp

Library for SRTP (Secure Realtime Transport Protocol)
Other
1.19k stars 469 forks source link

Cygwin compile broken #696

Open Thulinma opened 3 months ago

Thulinma commented 3 months ago

Hey there! Building on latest Cygwin (using Meson at least - but I assume other systems are affected too) doesn't work because machine/types.h and sys/int_types.h both exist on Cygwin, but are actually one and the same file. Both are detected and included by ./crypto/include/integers.h, causing an error due to redefinitions.

I fixed this with the following patch for myself (since I'm using libsrtp as a meson subproject, so patching is trivial):

diff --git a/meson.build b/meson.build
index 81a232e..1f15de9 100644
--- a/meson.build
+++ b/meson.build
@@ -43,6 +43,10 @@ foreach h : check_headers
   endif
 endforeach

+if (host_system == 'cygwin')
+  cdata.set('HAVE_MACHINE_TYPES_H', false)
+endif
+
 check_functions = [
   'sigaction',
   'inet_aton',

I opened this issue since I figured you might want to upstream that fix, and potentially also fix it in other build systems than just Meson (I assume the problem affects every build system, at least... but have not checked that 😇). And yes, I realize my "fix" is rather hacky, but it works and it's a very small change 🤷. Feel free to do it some other way of course! This issue was more intended to make you aware of the problem rather than acting as a PR (hence no PR and all that).

tp-m commented 3 months ago

No opinion on the correctness of the patch, but this could probably use a comment.

If they're the same file I would expect the headers to make sure double-includes work fine already tbh, perhaps this should be reported to the Cygwin folks as well?

pabuhler commented 3 months ago

@Thulinma I agree with @tp-m , it should be fine to include both, are there no include guards in place?

It should be noted that in the main branch integers.h has been removed, but maybe this problem has just moved to a different file now, would need to test. Regards integers.h is in the latest release so it would be good to address this. If the windows builds where enabled in .github/workflows/meson.yml then it should have help catch this earlier. Thanks for reporting it and we will see what we do with it, the patch seams a bit hackish but it is a start.

Thulinma commented 3 months ago

I figured it could use some changes, yes - I was in a hurry to "just get it working" for my own purposes 😅. (This is exactly why I only opened an issue with the information and didn't open a PR instead; to give you guys the room to solve this more cleanly than I did.)

And yeah, I'd expect a double include like that to have some sort of guards around it... my guess it might not be getting caught because it's two different paths for the same file and the compiler isn't detecting they are the same. I'll take a look at this soon-ish and will report to Cygwin as well if it looks like that makes sense. 👍