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

Cygwin compile broken #696

Open
Thulinma opened this issue Mar 25, 2024 · 3 comments
Open

Cygwin compile broken #696

Thulinma opened this issue Mar 25, 2024 · 3 comments

Comments

@Thulinma
Copy link

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
Copy link
Contributor

tp-m commented Mar 25, 2024

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
Copy link
Member

@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
Copy link
Author

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. 👍

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

No branches or pull requests

3 participants