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

Do we need to handle <stddef.h> specially? #4284

Closed
alejandro-colomar opened this issue May 2, 2024 · 1 comment · Fixed by #4289
Closed

Do we need to handle <stddef.h> specially? #4284

alejandro-colomar opened this issue May 2, 2024 · 1 comment · Fixed by #4289

Comments

@alejandro-colomar
Copy link
Member

alejandro-colomar commented May 2, 2024

- Regex: '<stddef\.h>'

The commit ac6ef83 claims to workaround a bug in sasl.h, which supposedly is missing an #include <stddef.h> for size_t.

However, I can see the include in my system:

alx@debian:~$ apt-file search sasl/sasl.h
libsasl2-dev: /usr/include/sasl/sasl.h    
alx@debian:~$ grep stddef /usr/include/sasl/sasl.h
#include <stddef.h>  /* For size_t */

And the include has been there since 2012, according to a git-blame(1) on the source repository:

alx@debian:/tmp/cyrus-sasl$ find | grep /sasl.h
./include/sasl.h
alx@debian:/tmp/cyrus-sasl$ git blame ./include/sasl.h | grep stddef
67a188693 (Ken Murchison           2012-12-20 18:14:50 -0500  124) #include <stddef.h>  /* For size_t */
$ git show 67a188693
commit 67a188693796a14e3a76ac603104807fbbfddfc4
Author: Ken Murchison <murch@andrew.cmu.edu>
Date:   Thu Dec 20 18:14:50 2012 -0500

    sasl.h: #include <stddef.h> for size_t on NetBSD

diff --git a/include/sasl.h b/include/sasl.h
index fef4d510..8b8a63fb 100755
--- a/include/sasl.h
+++ b/include/sasl.h
@@ -121,6 +121,8 @@
 #ifndef SASL_H
 #define SASL_H 1
 
+#include <stddef.h>  /* For size_t */
+
 /* Keep in sync with win32/common.mak */
 #define SASL_VERSION_MAJOR 2
 #define SASL_VERSION_MINOR 1

Are you sure we need commit ac6ef83 special-casing stddef.h?

And if we do, are you sure it's sasl's fault?

@flatcap
Copy link
Member

flatcap commented May 2, 2024

Dunno. Probably not.

Try:

  • Removing the rule from .clang-format
  • Running clang-format everything
  • Thoroughly test the build

If you push to a branch [devel/XYZ] most of the GitHub Actions will get run.
e.g. Build and Test, ASAN

There are a couple of other manual builds you'll need to check too:

  • FreeBSD (CirrusCI)
    Edit the file .cirrus.yml and delete lines 5,6 (only_if..., skip...)
    (that'll force it to trigger on push)
  • MacOS
    1. Go to the Actions Tab
    2. Click "Show more workflows"
    3. Select macOS
    4. Click "Run workflow" (right-hand-side)
    5. Select your branch

If all the Actions succeed, then the change is probably good enough.

alejandro-colomar added a commit that referenced this issue May 6, 2024
The commit ac6ef83 ("fix: build of imap/auth_sasl.c") claims to
workaround a bug in sasl.h, which supposedly is missing an
`#include <stddef.h>` for size_t.

However, I can see the include in my system:

	alx@debian:~$ apt-file search sasl/sasl.h
	libsasl2-dev: /usr/include/sasl/sasl.h
	alx@debian:~$ grep stddef /usr/include/sasl/sasl.h
	#include <stddef.h>  /* For size_t */

And the include has been there since 2012, according to a git-blame(1)
on the source repository:

	alx@debian:/tmp/cyrus-sasl$ find | grep /sasl.h
	./include/sasl.h
	alx@debian:/tmp/cyrus-sasl$ git blame ./include/sasl.h | grep stddef
	67a188693 (Ken Murchison           2012-12-20 18:14:50 -0500  124) #include <stddef.h>  /* For size_t */
	alx@debian:/tmp/cyrus-sasl$ git show 67a188693
	commit 67a188693796a14e3a76ac603104807fbbfddfc4
	Author: Ken Murchison <murch@andrew.cmu.edu>
	Date:   Thu Dec 20 18:14:50 2012 -0500

	    sasl.h: #include <stddef.h> for size_t on NetBSD

	diff --git a/include/sasl.h b/include/sasl.h
	index fef4d510..8b8a63fb 100755
	--- a/include/sasl.h
	+++ b/include/sasl.h
	@@ -121,6 +121,8 @@
	 #ifndef SASL_H
	 #define SASL_H 1

	+#include <stddef.h>  /* For size_t */
	+
	 /* Keep in sync with win32/common.mak */
	 #define SASL_VERSION_MAJOR 2
	 #define SASL_VERSION_MINOR 1

This seems to have been a red herring.  There was probably some include
problem somewhere else, and that change accidentally fixed it.

Or maybe back then in 2017, some distributions still had an already-very
old version of <sasl/sasl.h> which still didn't have the include.
However it is, the include has been there for more than a decade now,
and we don't need to workaround any bugs.

Fixes: ac6ef83 ("fix: build of imap/auth_sasl.c")
Link: <#4284>
Cc: Richard Russon <rich@flatcap.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit that referenced this issue May 6, 2024
Semi-scripted change:

	$ find * -type f \
	| grep '\.[ch]$' \
	| xargs grep -l 'include.*stddef' \
	| while read f; do
		clang-format -i "$f";
	done;

Plus manually removing unrelated changes.

Fixes: ac6ef83 ("fix: build of imap/auth_sasl.c")
Closes: <#4284>
Cc: Richard Russon <rich@flatcap.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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 a pull request may close this issue.

2 participants