-
Notifications
You must be signed in to change notification settings - Fork 440
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
base: master
Are you sure you want to change the base?
Fix Paho C build #471
Conversation
9533991
to
c764022
Compare
I was waiting to see if this PR would land in the Paho C repo before making any further changes to the build here. Some things in the master/develop branches assume it will land. |
OK sure. |
@laitingsheng @fpagliughi Can repeat it in VS code by issuing 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. |
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:
|
@laitingsheng
Worked for the mqtt Cpp cmake but the C lib cmake seemed to keep seeing it as false, having the cache worked.
|
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 |
.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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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}
.
There was a problem hiding this comment.
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.
@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 My one question is... what's the purpose of adding And if you notice that I missed anything, please let me know. |
This can avoid any unwanted targets in the subdirectory to be included in the implicit |
Thanks, @laitingsheng ! |
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.