Skip to content

Conversation

rshrj
Copy link
Contributor

@rshrj rshrj commented Mar 27, 2025

This PR fixes an issue in src/platform/CMakeLists.txt where add_compile_definitions() is used with a -D prefix:

add_compile_definitions(-DMULTIPASS_JOURNALD_ENABLED)

This results in a malformed macro definition -D-DMULTIPASS_JOURNALD_ENABLED, which causes a compiler error:

<command-line>: error: macro names must be identifiers

The fix is to pass only the macro name, as add_compile_definitions() automatically adds the -D prefix:

add_compile_definitions(MULTIPASS_JOURNALD_ENABLED)

Tested in a clean Debian 12 environment using Make, where this issue was originally triggered.

@xmkg xmkg self-requested a review March 27, 2025 12:07
@xmkg
Copy link
Member

xmkg commented Mar 27, 2025

Hi @rshrj, thanks for the contribution!

Good catch, I wonder why it hasn't caused any trouble before.

@xmkg
Copy link
Member

xmkg commented Mar 27, 2025

Hey @rshrj, could you sign the CLA using the same GitHub username and email when you get a chance?

@rshrj
Copy link
Contributor Author

rshrj commented Mar 27, 2025

Hey @rshrj, could you sign the CLA using the same GitHub username and email when you get a chance?

Done!

@rshrj
Copy link
Contributor Author

rshrj commented Mar 27, 2025

Hi @rshrj, thanks for the contribution!

Good catch, I wonder why it hasn't caused any trouble before.

Seems like MULTIPASS_ENABLE_SYSLOG is set true for most people

@xmkg
Copy link
Member

xmkg commented Mar 27, 2025

Seems like MULTIPASS_ENABLE_SYSLOG is set true for most people

Actually, it's the opposite: journald is the default logger for Linux.

According to the CMake documentation for add_compile_definitions, the command is supposed to strip the leading "-D" from the input, which is not happening in your case. I see that CMake 3.25.1 is the latest CMake version for the bookworm, so that figures.

It's a nice addition (or should I say removal) regardless :)

Copy link
Member

@xmkg xmkg left a comment

Choose a reason for hiding this comment

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

LGTM.

@github-merge-queue github-merge-queue bot closed this pull request by merging all changes into canonical:main in ab399ad Mar 28, 2025
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.

3 participants