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

[rmqcpp] Update to latest revision #40101

Merged
merged 8 commits into from
Sep 8, 2024
Merged

Conversation

SirWrexes
Copy link
Contributor

Fixes #38736

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.*
  • Only one version is added to each modified port's versions file.

*--overwrite-version was used, as this is still 1.0.0, minus a build failure.
I thought having a version like 1.0.1 or 1.0.0-1 could be a confusing divergence from the lib's versioning and didn't really make sense. Let me know if I should patch it with something different. 🫡

@SirWrexes SirWrexes force-pushed the port/rmqcpp branch 4 times, most recently from ab7e6c9 to b70f132 Compare July 25, 2024 22:26
@SirWrexes
Copy link
Contributor Author

SirWrexes commented Jul 25, 2024

About that version thing, I saw there are port versions and thought that would be the place to indicate the change. I wanted to go and edit that but I don't exactly know how this bit works (couldn't find docs about it or help in the CLI) so, scared of breaking things, I've rolled it back to an overwritten version.

@FrankXie05 FrankXie05 added the category:port-update The issue is with a library, which is requesting update new revision label Jul 26, 2024
@FrankXie05
Copy link
Contributor

@SirWrexes Thanks for posting this PR. You can refer to versioning for more information about the version. :)

@SirWrexes
Copy link
Contributor Author

@microsoft-github-policy-service agree

@SirWrexes
Copy link
Contributor Author

So, if I get this right, I should run vcpkg x-add-version --all --overwrite-version, then go edit the versions file to have port-version: 1, commit, push and we're good ? 🤔

@FrankXie05
Copy link
Contributor

@SirWrexes
Steps for version update:

  1. Modify the field version of file vcpkg.json to the latest version number.
  2. Delete the filded port-version of vcpkg.json.
  3. Run command .\vcpkg.exe install [port-name]:x64-windows to generate a new SHA and update the SHA in portfile.cmake.
  4. Submit a commit
  5. Run the command .\vcpkg.exe x-add-version [port-name] .
  6. Submit a commit.
  7. Push the code.

If it is not an updated port version, then:
Replace the second and fifth points with the following code
2. The value of port-version +1.
5. Run the command .\vcpkg.exe x-add-version [port-name] --overwrite-version .

@FrankXie05
Copy link
Contributor

@SirWrexes A simple example PR. Hope it helps you. :)
https://github.com/microsoft/vcpkg/pull/40024/files

@SirWrexes
Copy link
Contributor Author

SirWrexes commented Jul 26, 2024

Despite setting the port to checkout a more recent commit, the package in question indeed doesn't change version.
1.0.0 remains the targeted version, as this is the only one available, with the difference that with this fix it builds successfully—tested with an overlay. :)

After reading some more documentation (including the one you've link) and understanding the tooling better, I found I could set port-version inside the vcpkg.json of the port, which would in turn make sure that versions/baseline.json is updated accordingly, as well as versions/r-/rmqcpp.json, preventing any destructive operation while retaining the behaviour of making this fixed port the default for 1.0.0.

I'm sending the changes now.

@SirWrexes
Copy link
Contributor Author

That should do it. Thank you very much for your time and help !

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

This port has a lot of host dependencies which do not make any sense.

@SirWrexes
Copy link
Contributor Author

This port has a lot of host dependencies which do not make any sense.

Now that you mention it, yes, vcpkg-cmake and vcpkg-cmake-config look like the only ones that need to be set as host dependencies.
I tried applying the change and build went just fine.

Please disable tests in the vcpkg port. (#40101 (comment))

Regarding this part, I've checked their CMake files and... I don't think that's possible ?
image
src/tests/*/CMakeLists.txt do not provide a way to disable their corresponding targets either, and any attempt to use set_property(TEST|TARGET|DIRECTORY ...) or the equivalent set_target_properties(), set_directory_properties() etc with PROPERTY EXCLUDE_FROM_ALL 1 as vcpkg works in script mode and isn't directly attached to/aware of the configuration set in the ports' builds themselves.
This should be a PR for the lib itself rather than its port. 🤔

},
"boost-asio",
"boost-iostreams",
"gtest",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Despite only being needed for tests, gtest is set as a non-host dependency since there is no way to disable tests in the port for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test dirs are removed from the build. What is left?

Copy link
Member

Choose a reason for hiding this comment

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

Did you try building the port without this dependency? the patch above that deletes add_subdirectory(tests) from CMakeLists.txt should make it so that this dependency is no longer needed.

@dg0yt
Copy link
Contributor

dg0yt commented Jul 28, 2024

Please disable tests in the vcpkg port. (#40101 (comment))

Regarding this part, I've checked their CMake files and... I don't think that's possible ?

Then the simplest disabler is a patch which removes the add_subdirectory(tests).

@SirWrexes
Copy link
Contributor Author

Noted, will do when I get some free time (currently in a dev rush)! 🫡

@FrankXie05
Copy link
Contributor

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review".
That way, I can be aware that you've responded since you can't modify the tags.

@FrankXie05 FrankXie05 marked this pull request as draft July 30, 2024 02:24
@SirWrexes SirWrexes marked this pull request as ready for review August 8, 2024 10:23
@SirWrexes
Copy link
Contributor Author

Hello there
I finally got some time for myself and added the patch to disable building tests (and example binaries too).
Let me know if I should cleanup the commit log/squash everything down into just two commits.

versions/r-/rmqcpp.json Show resolved Hide resolved
@SirWrexes
Copy link
Contributor Author

I got mixed in my commands and pushed the wrong commit when amending last commit. I'm embarrassing myself... 🤦

@SirWrexes
Copy link
Contributor Author

Done. By the way, sorry for being messy about it, and thank you very much for your patience and understanding. 🫶

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Versioning seems to be complete strange now. Misleading review comments.

If version is not incremented, port-version must be incremented.
If version is incremented, port-version must be reset to 0.

But upstream can't have two different refs for the same release.

},
"boost-asio",
"boost-iostreams",
"gtest",
Copy link
Contributor

Choose a reason for hiding this comment

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

The test dirs are removed from the build. What is left?

@SirWrexes
Copy link
Contributor Author

SirWrexes commented Aug 10, 2024

Versioning seems to be complete strange now. Misleading review comments.

So... Setting port version to 1 was actually right ?

If version is not incremented, port-version must be incremented.
If version is incremented, port-version must be reset to 0.

That is what I read from the docs and is precisely why I up'd the port version.
Shall I revert again and re-do the thing with port version set to 1 then ?

@olejniczak
Copy link

Would it be possible to clarify what is missing to merge this PR?
@FrankXie05 requested a change, which was later judged by @dg0yt to be wrong.
@FrankXie05 can you withdraw the change request? Or give some comment?

@FrankXie05 FrankXie05 added the category:code-cleanup This PR cleans up code, without fixing any existing bugs nor adding any features. label Sep 4, 2024
@olejniczak
Copy link

@FrankXie05 Thank you!
@SirWrexes It seems to be ready to merge!

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Sep 5, 2024
@vicroms vicroms merged commit d211388 into microsoft:master Sep 8, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:code-cleanup This PR cleans up code, without fixing any existing bugs nor adding any features. category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rmqcpp] build failure
5 participants