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

PROTON-2216 Fix build of SASL related code on Windows #242

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

Conversation

josusky
Copy link

@josusky josusky commented Apr 27, 2020

The code that uses Cyrus SASL library failed to build on Windows because
of "pthead.h" include. There is for sure a nicer way how to make
locking in a platform independent way. I have tried to keep the
pthread code for Unix and have added a corresponding code for Windows.

The code that uses Cyrus SASL library failed to build on Windows because
of "pthead.h" include. There is for sure a nicer way how to make
locking in a platform independent way. I have tried to keep the
pthread code for Unix and have added a corresponding code for Windows.
@jiridanek
Copy link
Contributor

Windows build does not support using Cyrus SASL. Instead, it uses the stub, which supports SASL Anonymous and Plain.

# Skip the SASL detection on Windows.
# MSbuild scans output of Exec tasks and fails the build if it notices error-like
# strings there. This is a known issue with CMake and msbuild, see
# * https://github.com/Microsoft/msbuild/issues/2424
# * https://cmake.org/pipermail/cmake-developers/2015-October/026775.html
if cc.compiler_type!='msvc':
if cc.has_function('sasl_client_done', includes=['sasl/sasl.h'], libraries=['sasl2']):
libraries.append('sasl2')
sources.append(os.path.join(proton_src, 'sasl', 'cyrus_sasl.c'))
else:
log.warn("Cyrus SASL not installed - only the ANONYMOUS and PLAIN mechanisms will be supported!")
sources.append(os.path.join(proton_src, 'sasl', 'cyrus_stub.c'))
else:
log.warn("Windows - only the ANONYMOUS and PLAIN mechanisms will be supported!")
sources.append(os.path.join(proton_src, 'sasl', 'cyrus_stub.c'))

Jira: https://issues.apache.org/jira/browse/PROTON-2216

@josusky
Copy link
Author

josusky commented May 12, 2020

The Cyrus SASL library is multiplatform. I do not see a reason why Qpid-proton was restricting itself to less SASL methods on Windows platform.
Now when the JIRA issue is finally loaded, I can see that you have summed my intention in that issue [PROTON-2216] and added few more notes. What does it mean for me? Should I take any action?

@jiridanek jiridanek requested a review from astitcher May 12, 2020 11:32
@jiridanek
Copy link
Contributor

@josusky I don't know much about SASL and Cyrus SASL in Proton, so I cannot really review the PR, beyond the initial triage (calling this a new feature). I think nothing is required from you at this point.

If we want to support SASL on Windows, update to README.md or INSTALL.md is probably necessary, with build instructions. The code I linked above, in python/setup.py.in, is then also in need of update, I think. I won't ask that of you, though, because I am not certain how this PR is going to proceed.

@astitcher
Copy link
Member

@josusky Cyrus SASL may well be multiplatfrom, but there is no readily available of the library for Windows as far as I can tell. It is not available on vcpkg for instance and there is no windows package available from CMU.

I'm assuming that you are compiling it yourself - if not could you point to your source. If so, perhaps some brief compilation instructions or a pointer to them would be useful.

@jiridanek
Copy link
Contributor

jiridanek commented May 12, 2020

vcpkg request for cyrus sasl, microsoft/vcpkg#5894, currently abandoned; there is link to a random person providing Windows binaries

@astitcher
Copy link
Member

vcpkg request for cyrus sasl, microsoft/vcpkg#5894, currently abandoned

Unfortunately this is only an issue, rather than an actual PR with the changes necessary to actually build cyrus under vcpkg so it needs someone to do the work. However it does have a pointer to somebodies Windows port on sourceforge.

@josusky
Copy link
Author

josusky commented May 12, 2020

@astitcher Yes, we are building Cyrus SASL from source files. Please see https://github.com/cyrusimap/cyrus-sasl/tree/master/win32

@jiridanek jiridanek changed the title Fix build of SASL related code on Windows PROTON-2216 Fix build of SASL related code on Windows Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants