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

installed headers don't compile #4798

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

elliefm
Copy link
Contributor

@elliefm elliefm commented Jan 23, 2024

The Debian package maintainer privately reported a problem from the abi-compatibility-checker not being able to run due to the installed headers not compiling. This is part of Debian's work to switch to 64-bit time_t on some 32-bit platforms. Further details in https://cyrus.topicbox.com/groups/devel/T7b15dfc105949d4e-Meae606200efd965cc932318f

I'm using this PR to accumulate the fixes that we need to get the abi-compatibility-checker to actually run.

@elliefm elliefm force-pushed the v311/installed-headers-lib-prefix branch from 8eea16c to 77bf820 Compare January 23, 2024 23:35
@elliefm elliefm changed the title auth.h: installed headers can't #include "lib/..." debian abi-compatibility-checker can't run for cyrus-imapd Jan 23, 2024
@elliefm elliefm added backport-to-3.8 for PRs that are to be backported to 3.8 backport-to-3.10 for PRs that are to be backported to 3.10 labels Jan 23, 2024
@dilyanpalauzov
Copy link
Contributor

I thought your plan is to prefix in all #includes with a lib/, when appropriate. assert.h without lib/ can get very confusing and by tweaking CFLAGS replaced by assert from libc.

@elliefm
Copy link
Contributor Author

elliefm commented Jan 29, 2024

That was my plan (#3044), but until now I hadn't properly considered if or how these headers work when they're installed for use by third-party software that wants to link against libcyrus, and it turns out that's been broken by some of my changes in this direction.

Hopefully, cleaning it up enough for the abi-compatibility-checker to work will shake out the issues, and reveal a clear path forward.

@elliefm elliefm force-pushed the v311/installed-headers-lib-prefix branch 4 times, most recently from 417c56a to 7150603 Compare February 5, 2024 04:48
@@ -1,3 +1,5 @@
#include <config.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, from where comes this config.h file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's one of the outputs from running ./configure. It's not installed, and it can't be installed, so it's a mistake when the headers that are installed depend on it.

@dilyanpalauzov
Copy link
Contributor

That was my plan (#3044), but until now I hadn't properly considered if or how these headers work when they're installed for use by third-party software that wants to link against libcyrus, and it turns out that's been broken by some of my changes in this direction.

Hopefully, cleaning it up enough for the abi-compatibility-checker to work will shake out the issues, and reveal a clear path forward.

I do not understand how installing or not a header somewhere under /usr/include/cyrus/ is related to the Binary Interface compatibility. How do third party software stop working correctly, if Cyrus installs a .h file?

@elliefm
Copy link
Contributor Author

elliefm commented Feb 21, 2024

I do not understand how installing or not a header somewhere under /usr/include/cyrus/ is related to the Binary Interface compatibility.

It's not, really.

The Debian people needed to know whether the ABI would be compatible when they change some of their 32 bit platforms to use 64-bit time_t. They have a tool to check for this automatically, but it wants to compile the installed header files, which fails because some of them rely on stuff like config.h which isn't installed.

We've since established that the packages won't be ABI compatible, and so the Debian people will set their packages up to accommodate that. The tool no longer matters particularly... but the installed headers are still broken.

So it's not that the headers are a problem for ABI compatibility; they're a problem because they don't compile. It just so happens that it was the ABI compatibility checker that made us notice they don't compile.

@elliefm elliefm force-pushed the v311/installed-headers-lib-prefix branch 2 times, most recently from 9a3d4d2 to 0efb0e8 Compare February 22, 2024 23:23
@dilyanpalauzov
Copy link
Contributor

My understanding now is, that cyrus-imap installs header files, which in some way depend on conifg.h.

@elliefm elliefm removed backport-to-3.8 for PRs that are to be backported to 3.8 backport-to-3.10 for PRs that are to be backported to 3.10 labels Mar 6, 2024
When our headers are installed, they're no longer buried under a
"lib/" subdirectory, which means installed headers using this spelling
can't find their dependencies, and that means third party code that
wants to link against libcyrus et al cannot be compiled.

Installed headers must spell it `#include "foo.h"`, not `#include "lib/foo.h"`

md5.h is not  installed, so the spelling it uses should not make a difference.
xsha1.h is not installed, but ought to be.  I've fixed the spelling in both
of these for consistency
Needed by charset.h, which is installed.  Based on cyrusimap#4623, originally
by @dilyanpalauzov
That is: we can't install headers that depend on headers that aren't
installed
…cess env

don't ever assume the Cyrus install path is in the user's ld.so.conf
(and fix the name conflicts to make this possible)
this should probably be in Libs.private, but example_libcyrus.c needs
libcyrus_min because it calls config_read, and i'm pretty sure it has
to call config_read, and so it should really link against libcyrus_min
itself rather than relying on libcyrus to bring it in, but that
complicates the LibCyrus.pm setup, which would need to know about the
multiple dependencies...

----

actually, i think the above comment is bogus, and this commit is
correct.  pc(5) says Libs.private is for static linking, not for
differentiating a library's own dependencies from those a caller
needs

need to check the other libfoo.pc.in files and make them all coherent...
... rather than from file name, so that examples can easily
express multiple dependencies
@elliefm elliefm force-pushed the v311/installed-headers-lib-prefix branch from 0efb0e8 to 07a3120 Compare April 26, 2024 01:18
@elliefm elliefm changed the title debian abi-compatibility-checker can't run for cyrus-imapd installed headers don't compile Apr 26, 2024
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

Successfully merging this pull request may close these issues.

None yet

3 participants