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

Fix Paho C build #471

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix Paho C build #471

wants to merge 1 commit into from

Conversation

laitingsheng
Copy link

@laitingsheng laitingsheng commented Dec 31, 2023

This PR is mainly for fixing the CMake error when PAHO_WITH_MQTT_C is enabled.

I also changed the Git submodule URL to a relative path so that it can follow the same protocol used to clone the main repository.

Fixes #469.

@laitingsheng laitingsheng changed the title Use relative path for Git submodules Fix Paho C build Dec 31, 2023
@fpagliughi
Copy link
Contributor

I was waiting to see if this PR would land in the Paho C repo before making any further changes to the build here.
eclipse/paho.mqtt.c#1428

Some things in the master/develop branches assume it will land.

@laitingsheng
Copy link
Author

OK sure.

@Zelif
Copy link

Zelif commented Jan 14, 2024

@laitingsheng @fpagliughi
This still seems to fail for the first clean cmake build with the same error in the issue.

Can repeat it in VS code by issuing
CMake: Delete cache and Reconfigure

Ends up causing issues inside github actions as it is a clean build every time. Will try to figure out why the C lib is not found on the alias and install step for the first time.

@laitingsheng
Copy link
Author

laitingsheng commented Jan 15, 2024

It works fine on Ubuntu (with or without OpenSSL). What error output did you get?

FYI, I am using this command to generate the build folder:

cmake --fresh -G Ninja -B build -S . -DPAHO_WITH_SSL:BOOL=TRUE -DPAHO_WITH_MQTT_C:BOOL=TRUE

@Zelif
Copy link

Zelif commented Jan 15, 2024

@laitingsheng
Sorry for my derp, this seems to be a result of how I was setting the SSL variable in the cmake
Having the top level cmake set this:

SET(PAHO_WITH_SSL					ON)

Worked for the mqtt Cpp cmake but the C lib cmake seemed to keep seeing it as false, having the cache worked.

SET(PAHO_WITH_SSL					ON CACHE BOOL "Enable SSL build")

@laitingsheng
Copy link
Author

laitingsheng commented Jan 15, 2024

@Zelif

Maybe you can have a look at this policy: https://cmake.org/cmake/help/latest/policy/CMP0077.html

And you can probably set this policy variable prior to the PAHO_WITH_SSL: https://cmake.org/cmake/help/latest/variable/CMAKE_POLICY_DEFAULT_CMPNNNN.html

.gitmodules Outdated
@@ -1,3 +1,3 @@
[submodule "src/externals/paho-mqtt-c"]
path = src/externals/paho-mqtt-c
url = https://github.com/eclipse/paho.mqtt.c.git
url = ../paho.mqtt.c.git
Copy link

Choose a reason for hiding this comment

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

Does this break forks for people who have only the CPP version forked?
The solution to it is just to fork the c lib yourself. It was trying to pull down my own fork which I had not done.

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to ../../eclipse/paho.mqtt.c.git instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Zelif is correct. The main point of using the submodule is so that people can just grab the Paho C++ code and build it without having to think or worry about the C lib.

That would mean grabbing the C lib directly off of GitHub... not from a local directory! It would be wrong to assume that the C library sources have been downloaded locally.

Copy link
Author

@laitingsheng laitingsheng Jun 19, 2024

Choose a reason for hiding this comment

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

No. ../.. refers to github.com, and it will use the same protocol as the parent module. This is to avoid using https submodule even if you clone the parent repository with ssh.

Copy link
Author

@laitingsheng laitingsheng Jun 19, 2024

Choose a reason for hiding this comment

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

The reason .. doesn't work when you fork it is that the forked repository is in your own namespace, the full repository path of the forked repository will be github.com/{username}/paho.mqtt.cpp, and .. will refer to github.com/{username}.

Copy link
Author

@laitingsheng laitingsheng Jun 19, 2024

Choose a reason for hiding this comment

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

https://git-scm.com/docs/git-submodule#_commands
You can have a look at the doc about submodule relative URL.

@fpagliughi
Copy link
Contributor

fpagliughi commented Jun 15, 2024

@laitingsheng , thanks so much for this PR. I gave up on waiting for the upstream library release, and started hacking on the existing build, forgetting that this was already here. I think I covered most of this in my changes, but sometimes in slightly different ways, like I meant to push externals/ up to the top-level directory, and I definitely want to keep the submodule referring to GitHub, etc.

My one question is... what's the purpose of adding EXCLUDE_FROM_ALL to the add_subdirectory() call for the C lib code?

And if you notice that I missed anything, please let me know.

@laitingsheng
Copy link
Author

My one question is... what's the purpose of adding EXCLUDE_FROM_ALL to the add_subdirectory() call for the C lib code?

This can avoid any unwanted targets in the subdirectory to be included in the implicit all target.

@fpagliughi
Copy link
Contributor

Thanks, @laitingsheng !
I will read up on all of this.

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.

Build with PAHO_WITH_MQTT_C is broken
3 participants