-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
bugfix paho-mqtt-c: remove force dynamic linking since there is a check anyway #3382
Conversation
For the sake of context: https://cpplang.slack.com/archives/C41CWV9HA/p1604327707394000 Probably a bug in Conan when using lockfiles and not a recipe problem. @solvingj is investigating this 😄 |
here the lock file , but , the default option to be shared on non Windows forms is wrong anyway .... here the linke to the comment that mentions the problem, so the default should only be on Windows. Explicit! So I will update the PR to set this option only on Windows
|
just to extract the info from the previous post, so it does not get lost: here the linke to the comment that mentions the problem, the default for shared is required on Windows. Explicit! so imho still a something that should be fixed in the recipe, because on non Windows platforms, static should still be the default |
The current PR appears to me to be the right thing to do. Just move the windows-default inside the windows condition. I am still trying to understand how a lockfile was created successfully with a list of settings and options, but throws an exception when trying to use that lockfile. In theory, the lockfile creation process has to execute the |
just by running conan install with profiles that set the option, @solvingj conan version is 1.31.0 this is the install , then using the lockfile from the install generates the problem
|
All green in build 2 (
|
Ok, I THINK I get it now. I will report a new bug based on this. |
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 don't think this should get merged. Having different default options based on the OS is even more confusing. And again, if I recall correctly and understood @icraggs correctly, then paho-mqtt-c did not official support static linking before 1.3.4
I provided a link, static linking was not working for Windows for one person, and this was not by the author. Read it @Croydon If Windows needs special handing, why infecting the working and sane solutions and OSes? I seriously do not think blocking this PR and preventing a general well working solution is productive and helpful. |
I did read it several times and even participated in the review back then. @icraggs said that static linking wasn't tested and wasn't intended to be used in older versions. He didn't limit this statement to Windows-only. See e.g. #1888 (comment) And to repeat this argument, I think from a consumer perspective it is much more surprising that by default Windows builds shared libraries and other operating systems build static. Having this uniform might prevent unpleasant surprises or even more conditional handling on the consumer side.
I can't block pull requests. If other reviewers think the benefits of this change are bigger than the disadvantages and you get two more approvals it will get merged.
The current state should work just fine. The problem you run into is a Conan client bug and I'm somewhat wondering if you would have wanted to change the recipe if you wouldn't have encountered the client bug. |
I agree that this PR solves the issue but I also believe this PR was encouraged due to the Conan issue, so let's wait for the ongoing investigation of this issue at conan-io/conan#7991. Thank you |
The aforementioned bug has been resolved, could this be retested to see if the issue is solved without this PR? |
I detected other pull requests that are modifying paho-mqtt-c recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
I think this is not required anymore with conan 1.32, |
Specify library name and version: paho-mqtt-c/1.3.x
The removed lines caused a problem when using lock files
since there is a check later anyway, and forcing surprising options should not happen anyway, I think removing this is the best option.
Alternative, it could be intended, so setting shared happens only on Windows, but personally do not find that every exciting
conan-center hook activated.