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

Release 2.4.8 #4701

Draft
wants to merge 3 commits into
base: release/2.4
Choose a base branch
from
Draft

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Dec 10, 2024

Release 2.4.8

Refactor:

  • Add namespace
  • Enable sanitize builds
  • Refactor platform unittest external cmake
  • Add alias

- Add namespace
- Enable sanitize builds
- Refactor platform unittest external cmake
@talregev talregev requested a review from a team as a code owner December 10, 2024 14:14
@talregev talregev changed the title Release 2.4.* Release 2.4.8 Dec 11, 2024
@@ -272,7 +272,7 @@ configure_file(msquic-config.cmake.in ${CMAKE_BINARY_DIR}/msquic-config.cmake)
install(FILES ${CMAKE_BINARY_DIR}/msquic-config.cmake DESTINATION share/msquic)

if(BUILD_SHARED_LIBS)
install(EXPORT msquic DESTINATION share/msquic)
install(EXPORT msquic NAMESPACE msquic:: DESTINATION share/msquic)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy seeing CMake namespaces being added to exported targets. But it should be clear that this is a breaking change if not complemented by providing an alias with the old name. I wonder if a breaking change is desired in a patch release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already submit it to upstream and it accepted. and maybe it will be release.
#4692
#4701

Copy link
Contributor

Choose a reason for hiding this comment

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

This repo is upstream. That's why I raise the point:
Adding the namespace without add polyfill breaks users.

Copy link
Member

Choose a reason for hiding this comment

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

How do we add the alias? I assume this is only a build time breaking change? Regardless, I would like to prevent those, if possible.

Copy link
Contributor

@dg0yt dg0yt Dec 11, 2024

Choose a reason for hiding this comment

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

The namespace is a useful service for consumers, but it doesn't affect the build: It signals to CMake that the identifier is a CMake target, not plain input to the linker. This improves detecting configuration errors.

Alias could be setup in msquic-config.cmake, after including the generated export files. Sth. like

foreach(_t IN ITEMS msquic msquic_platform ...)
    if(TARGET msquic::${_t} AND NOT TARGET ${_t})
        add_library(${_t} ALIAS msquic::${_t})
    endif()
endforeach()

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be removed in 2.5 or 3, after proper deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dg0yt I thought you are comment on vcpkg.
Can you add alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dg0yt How you can add this to msquic-config.cmake? this file generate automatic. I will glad for your help.

@talregev
Copy link
Contributor Author

@nibanks @dg0yt
I don't have the knowledge how to add alias. I am close this PR.

@talregev talregev closed this Dec 11, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Dec 12, 2024

The suggested change was given almost literally. (I didn't fill in all targets.)
The change is to be inserted after this line:

include(${SELF_DIR}/msquic.cmake)

Are there any other obstacles?

@talregev
Copy link
Contributor Author

This should be done in separate PR to the main.

@talregev talregev reopened this Dec 13, 2024
@talregev
Copy link
Contributor Author

@nibanks
I added the alias

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.82%. Comparing base (622a725) to head (9be6488).

Additional details and impacted files
@@               Coverage Diff               @@
##           release/2.4    #4701      +/-   ##
===============================================
- Coverage        86.86%   86.82%   -0.05%     
===============================================
  Files               56       56              
  Lines            17338    17338              
===============================================
- Hits             15061    15053       -8     
- Misses            2277     2285       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@talregev talregev closed this Dec 14, 2024
@talregev talregev reopened this Dec 17, 2024
@talregev talregev requested a review from nibanks December 17, 2024 11:30
@talregev talregev marked this pull request as draft December 24, 2024 15:56
@talregev
Copy link
Contributor Author

Better wait to @dg0yt changes in vcpkg:
microsoft/vcpkg#42788

@talregev talregev closed this Dec 27, 2024
@talregev talregev deleted the TalR/release/2.4 branch December 27, 2024 17:54
@talregev talregev restored the TalR/release/2.4 branch December 30, 2024 20:22
@talregev talregev reopened this Dec 30, 2024
@talregev
Copy link
Contributor Author

@nibanks At the moment, vcpkg port was merge without apply the patch inside.
Do you want to move forward with these changes that will match more closer msquic head to release?

Let me know what do you think.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 31, 2024

IMO one PR shouldn't mix these independent topics:

  • Bumping version number
  • Adding CMake export namespace and aliases
  • Adding a test for external CMake usage.

The PR which adds the test could also add commits with necessary fixes. That's how to get to a good CMake usage pattern. Similar to what was done in vcpkg with vcpkg-ci-msh3.

Dealing with export namespace and aliases would follow the integration of the test.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 31, 2024

Dealing with export namespace and aliases would follow the integration of the test.

Probably it is enough to add an alias for msquic because this is the only target name which was exported in past releases IIUC.

@talregev
Copy link
Contributor Author

talregev commented Dec 31, 2024

Dealing with export namespace and aliases would follow the integration of the test.

Probably it is enough to add an alias for msquic because this is the only target name which was exported in past releases IIUC.

the commits are cherry picks. As upstream rules, you just bring previous commits from head without change them. If I understood it correctly.

I have one commit that bring the namespace and refactor platform unittest external cmake. (There is no adding test, it old text).
And second commit is the alias.
and the third is bump the version number (without cherry picks).

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.

3 participants