cyrusimap / cyrus-sasl

Other
131 stars 150 forks source link

MSVC attribute handling from perspective of packaging and public includes #831

Closed adamws closed 2 months ago

adamws commented 5 months ago

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

https://github.com/cyrusimap/cyrus-sasl/blob/ef0bafd15f5a541033c673e0e68993b07c16147a/include/saslplug.h#L107

I suppose this is why this exist:

https://github.com/cyrusimap/cyrus-sasl/blob/ef0bafd15f5a541033c673e0e68993b07c16147a/win32/include/config.h#L137-L138

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 commented 2 months ago

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

hyc commented 2 months ago

Ideally autoconf should test if __attribute__ is supported by the compiler. There used to be a test for this, but it was removed in 2e4af8049fd397f618d55142cea4774af404c6e8 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