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

poco: Patch for CMakeLists.txt to include pocomsg.rc #26093

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

KevDi
Copy link

@KevDi KevDi commented Dec 3, 2024

Summary

Changes to recipe: poco/1.13.3 (poco/1.14.0)

Motivation

Currently when building the Poco Library the generated Resource File pocomsg.rc is not included in the build of the DLL. This leads to warnings in the Windows Eventviewer if the Poco::Logger is used with the Poco::EventLogChannel.

Details

With this Patch the pocomsg.rc is added to the Sources of the Poco::Foundation DLL. Afterwards the Log Messages will be displayed correctly in the Windows EventViewer. I already opened a PR in the Poco Project and it was approved for the next Bugfix Release (1.14.1): pocoproject/poco#4803
Maybe this PR should be handled before updating the Receipe to 1.14.0 ( #26090 )


  • Read the contributing guidelines
  • Checked that this PR is not a duplicate: list of PRs by recipe
  • Tested locally with at least one configuration using a recent version of Conan (Tested all the Versions provided in the config.yml

Currently the generated Resource pocomsg.rc is not added to the
builded DLL. This leads to warning messages (M in the Windows Eventviewer,
if the `Poco::Logger` is configured to use the `EventLogChannel`.
@KevDi KevDi changed the title Patch for CMakeLists.txt to include pocomsg.rc poco: Patch for CMakeLists.txt to include pocomsg.rc Dec 3, 2024
@jcar87 jcar87 self-assigned this Dec 3, 2024
@ErniGH
Copy link
Contributor

ErniGH commented Dec 10, 2024

Hello @KevDi! First of all, thank you so much for your effort and your pull request. I wanted to share a couple of thoughts. I think it’s better not to overwrite patches, as it helps maintain better traceability in the future. Also, your pull request to upstream looks excellent, and it seems they've added it to the next milestone. However, I believe it’s best to wait for that PR to be merged before merging these patches and backporting all versions to Conan Center. Let me know what you think!

@KevDi
Copy link
Author

KevDi commented Dec 10, 2024

@ErniGH sounds like a plan for me. So by overwriting Patches you mean it would be better to create a seperate Patch File for this change?

@ErniGH
Copy link
Contributor

ErniGH commented Dec 10, 2024

@KevDi Exactly! And add them to conandata.yml. 😄

@KevDi
Copy link
Author

KevDi commented Dec 11, 2024

Ok i will update the PR around next week.

@KevDi
Copy link
Author

KevDi commented Dec 17, 2024

@ErniGH is there a guide how to properly create the patches? I tried with git diff but i currently getting the error patch stream is incomplete even for the file that i have submitted here. I really don't get it why i got this error.

@ErniGH
Copy link
Contributor

ErniGH commented Dec 18, 2024

Hello @KevDi, Unfortunately, we don't have a guide for making these patches. It is indeed done with a git diff, but you must apply the existing patches with git apply before making your changes. We know it's a limitation, and we are thinking about how to modify it in the future.

Best regards

@KevDi
Copy link
Author

KevDi commented Dec 19, 2024

@ErniGH Thanks for the reply. I managed to get it working and updated the PR.

@ErniGH
Copy link
Contributor

ErniGH commented Dec 23, 2024

@KevDi , thank you very much for the changes. I've modified a small detail to avoid noise in the changelog. We'll wait for upstream to merge the changes 😄

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.

4 participants