Skip to content

Conversation

@mxmanghi
Copy link
Contributor

Change declarations that break C23 standards. I took the liberty of renaming configure.in as configure.ac (as now customary with autotools) and also removed an unused directory from CFLAGS include arguments. Now the source code relies on config.h to define the PACKAGE_VERSION symbol

…onfigure.ac, removed unused directory from CFLAGS include arguments
@bovine bovine requested a review from resuna February 19, 2025 19:47
@resuna
Copy link
Member

resuna commented Feb 19, 2025

Tests failing here:

🍺  /opt/homebrew/Cellar/yajl/2.1.0: 19 files, 260.4KB
ln: /usr/local/include/tcl8.6: No such file or directory
Error: Process completed with exit code 1.

@gahr
Copy link
Contributor

gahr commented Feb 19, 2025

Tests failing here:

🍺  /opt/homebrew/Cellar/yajl/2.1.0: 19 files, 260.4KB
ln: /usr/local/include/tcl8.6: No such file or directory
Error: Process completed with exit code 1.

Please rebase, this should be fixed by #44.

@mxmanghi
Copy link
Contributor Author

As per Jeff's suggestion CONST keywords were turned into standard const. I also defined MODULE_SCOPE in configure.ac in order to avoid a conflicting definition in Tcl9 but a clearly suboptimal solution

@bovine
Copy link
Contributor

bovine commented Feb 23, 2025

The Mac CI seems to be failing to find a yajl include:

In file included from ./generic/yajltcl.c:6:
./generic/yajltcl.h:14:10: fatal error: 'yajl/yajl_version.h' file not found
#include <yajl/yajl_version.h>

@mxmanghi
Copy link
Contributor Author

The package doesn't build on Debian/Ubuntu and they are close to the pre-release freeze. If you want to take your time before integrating this pull request in a new bugfix release let me know. In that case I will produce a patch for the Debian package that fixes the C23 standards issue

# yajl incorrectly specified $(prefix)/include/yajl in pkg-config. This
# has been reported upstream as https://github.com/lloyd/yajl/pull/139.
# Until it's fixed, we work around it and get to the parent directory.
INCLUDES = @PKG_INCLUDES@ @TCL_INCLUDES@ $(YAJL_CFLAGS) $(YAJL_CFLAGS)/..
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the cause of the failure to build on mac. See the comment just above of this line. Why are you removing the fix?

Copy link
Contributor Author

@mxmanghi mxmanghi Feb 27, 2025

Choose a reason for hiding this comment

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

Even though the compilation flawlessly works on linux the options come with an -I /.. because YAJL_CFLAGS is an empty string. I guess the answer lies in a proper way to address the ticket quoted in the comments...the current status of the fix may be harmless for Linux but if we manage to have consistent option argument values it's safer (why tinkering with a non existing and incongruous '/..' directory after all?). Maybe we should let 'configure' build the YAJL_CFLAGS consistently with all the switches

-- M

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you end up with an empty YAJL_CFLAGS, given this?

yajl-tcl/configure.in

Lines 191 to 192 in b737336

PKG_CHECK_MODULES([YAJL], [yajl >= 2.0], [],
[AC_MSG_ERROR([Cannot find yajl])])

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with fixing the pkg-config output earlier in configure. Can you please put the fix back until we do so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It's part of this conversation now

Revert symbol INCLUDE modification while the issue with the YAJL_CFLAGS is solved by fixing pkg-config
@bovine
Copy link
Contributor

bovine commented Feb 27, 2025

The CI is still failing on Mac. Do we want to fix those before merging this?

@bovine bovine merged commit 5cec890 into flightaware:master Mar 28, 2025
1 of 2 checks passed
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.

4 participants