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 attribute handling from perspective of packaging and public includes #831

Closed
adamws opened this issue Apr 17, 2024 · 2 comments
Closed
Assignees
Milestone

Comments

@adamws
Copy link

adamws commented Apr 17, 2024

The saslplug.h header uses __attribute__ which is not handled by msvc:

void (*log)(sasl_conn_t *conn, int level, const char *fmt, ...) __attribute__((format(printf, 3, 4)));

I suppose this is why this exist:

/* we're not gcc */
#define __attribute__(foo)

The problem is that config.h is not part of installation and saslplug.h is, meaning that when packaged with conan for example, msvc users would get header which is not immediately usable.
I tested that by downloading cyrus-sasl from conancenter:

conan download 'cyrus-sasl/2.1.28#' -r conancenter --package-query="os=Windows" 

and checking how it is packaged:

$ tree
.
├── bin
│   └── sasl2.dll
├── conaninfo.txt
├── conanmanifest.txt
├── include
│   └── sasl
│       ├── exits.h
│       ├── gai.h
│       ├── hmac-md5.h
│       ├── md5global.h
│       ├── md5.h
│       ├── prop.h
│       ├── sasl.h
│       ├── saslplug.h
│       └── saslutil.h
├── lib
│   └── sasl2.lib
└── licenses
    └── COPYING

I think that there is a room for improvement here, i.e. public headers should contain platform-specific attribute handling.

@hyc
Copy link
Contributor

hyc commented Jul 23, 2024

Note - the same situation exists with mac/include/config.h. That all seems to be obsolete though, ever since MacOSX was released.

@hyc
Copy link
Contributor

hyc commented Jul 23, 2024

Ideally autoconf should test if __attribute__ is supported by the compiler. There used to be a test for this, but it was removed in 2e4af80 because redefining __attribute__ would break libc.

The correct fix would be to replace all uses of __attribute__ in the source with a Cyrus-SASL specific macro, so we can define it at will without affecting any other libraries. That's rather invasive but I think it's the right way to go. Also instead of the obsolete CMU-specific test, we can use autoconf's own test https://www.gnu.org/software/autoconf-archive/ax_c___attribute__.html

@hyc hyc closed this as completed in 06f41c4 Jul 24, 2024
dmoody256 pushed a commit to mongodb-forks/cyrus-sasl that referenced this issue Aug 1, 2024
It's a compiler-specific feature and we can't depend on other
compilers supporting it identically or at all.
Fix cyrusimap#831.

Signed-off-by: Howard Chu <hyc@symas.com>
(cherry picked from commit 06f41c4)
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