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.

Sign up for GitHub

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

Jump to bottom

[paho-mqtt-c] Explicit lib list + remove excessive build system patch #4356

Merged
merged 22 commits into from
Apr 26, 2021
Merged

[paho-mqtt-c] Explicit lib list + remove excessive build system patch #4356

merged 22 commits into from
Apr 26, 2021

Conversation

prince-chrismc
Copy link
Contributor

@prince-chrismc prince-chrismc commented Jan 25, 2021 •
edited
Loading

Specify library name and version: paho-mqtt-c/all

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

This is a follow up to recent issues #4096 #4063 #4293

Implemented #4063 (comment)

I tested ~8 combinations and it seems to be working with the patches removed... The built times are tiny so so I remove them completely?

@github-actions github-actions bot mentioned this pull request Jan 25, 2021
Open
@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 25, 2021 •
edited
Loading

Honestly, I don't like to build or package unecessary libs. If for maintenance purpose, the patch is removed, I would prefer useless files to be removed in package().

Eventually build a specific target, and do manual copy of public headers and libs. Or even better, submit a PR to upstream (add options to build specific targets or all of them).

@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@prince-chrismc
Copy link
Contributor Author

I would prefer useless files to be removed in package().

I think is a good idea, I am going to try it...

@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@prince-chrismc
Copy link
Contributor Author

Is it possible that the test_packge is skipping the config_options() ? it should be forcing it to shared but it keeps looking for static

@conan-center-bot

This comment has been minimized.

Sign in to view
@prince-chrismc
Copy link
Contributor Author

derp

@conan-center-bot

This comment has been minimized.

Sign in to view
@prince-chrismc
Copy link
Contributor Author

😖

-- OpenSSL hints: C:/OpenSSL-Win64
-- OpenSSL headers found at C:/J/w/cci_PR-4356@3/.conan/data/openssl/1.1.1i/_/_/package/963bb116781855de98dbb23aaac41621e5d312d8/include
-- OpenSSL library found at OPENSSL_LIB-NOTFOUND
-- OpenSSL Crypto library found at OPENSSLCRYPTO_LIB-NOTFOUND
-- Configuring incomplete, errors occurred!
See also "C:/J/w/cci_PR-4356@3/.conan/data/paho-mqtt-c/1.3.0/_/_/build/2d86b1c0b604546553f5dfd7c69f2005ea37d909/CMakeFiles/CMakeOutput.log".
paho-mqtt-c/1.3.0:

@a4z
Copy link
Contributor

a4z commented Feb 6, 2021

I admire your stamina, @prince-chrismc, but I wonder if you have meanwhile similar negative feelings for this recipe/package as I do ....

It's really bad that the upstream project is not able to provide a useful build, this as wasted provable so many time of many people here ....
all that should be fixed in the project itself, and there should be gh actions in the paho-mqtt-c project that tests the builds on the major platforms. But I do not think that the people really have the know how to do so, Too bad there is no good/better alternative ....

@conan-center-bot

This comment has been minimized.

Sign in to view
@prince-chrismc
Copy link
Contributor Author

That's a static ssl obj with fPIC always ON.... https://github.com/eclipse/paho.mqtt.c/blob/64a5ff3c3b71fe019353aeacaebc66a3cf4f3461/src/CMakeLists.txt#L249

Is this a problem?

@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
prince-chrismc and others added 5 commits April 17, 2021 20:47
@prince-chrismc
[paho-mqtt-c] explicit lib list
4c0c62a
@prince-chrismc @SpaceIm
Update recipes/paho-mqtt-c/all/conanfile.py
4d8d8e6 
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
@prince-chrismc
zap build patch + rm extra libs from package
5226782 
unfortunately this causes it to build all 4 combinations but the build time is very short
@prince-chrismc
Test to see if the package_info does not account for cofig_options
08c5f79
@prince-chrismc
Update conanfile.py
cf95ce4
@prince-chrismc @SpaceIm
warn + del samples
3a41abc 
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
@conan-center-bot

This comment has been minimized.

Sign in to view
a4z
a4z previously approved these changes Apr 20, 2021
View reviewed changes
@prince-chrismc prince-chrismc changed the title [paho-mqtt-c] explicit lib list [paho-mqtt-c] Explicit lib list + remove excessive build system patch Apr 21, 2021
@prince-chrismc
Copy link
Contributor Author

👋 @uilianries @SSE4 @madebr @ericLemanissier if guys have a second please take a look at this PR 🙏

@conan-center-bot

This comment has been minimized.

Sign in to view
SSE4
SSE4 previously approved these changes Apr 21, 2021
View reviewed changes
@SSE4 SSE4 requested a review from uilianries April 21, 2021 13:16
@prince-chrismc prince-chrismc dismissed stale reviews from SSE4 and a4z via f61a8a0 April 22, 2021 18:04
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot
Copy link
Collaborator

All green in build 20 (f61a8a0b0c4171d8935fc5047c714b6761343346):

  • paho-mqtt-c/1.3.1@:
    All packages built successfully! (All logs)

  • paho-mqtt-c/1.3.0@:
    All packages built successfully! (All logs)

  • paho-mqtt-c/1.3.4@:
    All packages built successfully! (All logs)

  • paho-mqtt-c/1.3.5@:
    All packages built successfully! (All logs)

  • paho-mqtt-c/1.3.6@:
    All packages built successfully! (All logs)

  • paho-mqtt-c/1.3.8@:
    All packages built successfully! (All logs)

SSE4
SSE4 approved these changes Apr 24, 2021
View reviewed changes
Copy link
Contributor

@SSE4 SSE4 left a comment

Choose a reason for hiding this comment

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

less patches - good

SpaceIm
SpaceIm approved these changes Apr 26, 2021
View reviewed changes
Copy link
Contributor

@SpaceIm SpaceIm left a comment

Choose a reason for hiding this comment

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

Not completly convinced by the resulting packages (headers specifically).

@conan-center-bot conan-center-bot merged commit 4316ec9 into conan-io:master Apr 26, 2021
@prince-chrismc
Copy link
Contributor Author

I agree it's not perfect. But checked the git blame and all the expected files are there.

We could hardcode the list of files but that more likely to break then the pattern.

@a4z
Copy link
Contributor

a4z commented Apr 27, 2021

Great work Chris, thanks a lot!

About the header for the install, this should of course be handled by the project, if a simple make install DESTDIR=/where/ever doesn't provide the right files, it is hard and will constantly require maintenance, when public headers are added or changed.
I wish the maintainer of this library would show similar activity when it comes to fixing and dealing with the situation in the build as he did in other situations.
But anyhow, I am happy to see here highly skilled and gifted people dealing with this situation and bring improvements in a range that is possible much better than I could, thanks again!

AlvaroFS pushed a commit to AlvaroFS/conan-center-index that referenced this pull request May 7, 2021
@prince-chrismc @SpaceIm @AlvaroFS
(conan-io#4356) [paho-mqtt-c] Explicit lib list + remove excessive bu…
f338160 
…ild system patch

* [paho-mqtt-c] explicit lib list

* Update recipes/paho-mqtt-c/all/conanfile.py

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>

* zap build patch + rm extra libs from package

unfortunately this causes it to build all 4 combinations but the build time is very short

* Test to see if the package_info does not account for cofig_options

* Update conanfile.py

* fix incorrect throw for invalid config

* use remove by mask

* fix patch name

* refactor epl file

* bump openssl

* del fpic when shared

* handle bin folder for windows shared

* tweak matchers

* bump openssl + fix path

* test: build target + selective install

* patch openssl lookup

might be required for 1.3.0

* more limted selection on copy

* zap pdbs

* try re-using the patch since it looks the same

* respect fPIC option

* warn + del samples

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>

* selective copy of headers

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SpaceIm SpaceIm SpaceIm approved these changes

@a4z a4z a4z approved these changes

@madebr madebr madebr approved these changes

@SSE4 SSE4 SSE4 approved these changes

@uilianries uilianries Awaiting requested review from uilianries

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@prince-chrismc @SpaceIm @conan-center-bot @a4z @SSE4 @madebr