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

export platform #4574

Merged
merged 1 commit into from
Nov 7, 2024
Merged

export platform #4574

merged 1 commit into from
Nov 7, 2024

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Sep 28, 2024

Description

For support compile msquic with external msh3 version 0.7.0. It require symbol that found on platform target.
I export these symbols

I rename the target platform to msquic_platform, because when platform is exported it create a lib file. Create a lib file with generic name will interfere with other libraries when install together. When the name is renamed to specific name it will allow to integrate more easy on libraries managers such as vcpkg.

Other info

fix #4569

@talregev talregev requested a review from a team as a code owner September 28, 2024 03:44
@talregev talregev marked this pull request as draft September 28, 2024 03:47
@talregev talregev marked this pull request as ready for review September 28, 2024 03:48
@nibanks nibanks added Area: Build external Proposed by non-MSFT labels Sep 28, 2024
src/bin/CMakeLists.txt Outdated Show resolved Hide resolved
@talregev
Copy link
Contributor Author

This is a draft PR and need more work done for it.
Issue in this PR (maybe more)

  • It create platform.lib. this is a generic name that can collide with other libs. I propose to change target platform to msquic_platform
  • I export openssl targets, but there other option that it use schannel, or osx ssl lib. need to do it more generic

@nibanks can you help me to solve these issue in this PR?

@talregev talregev force-pushed the TalR/export_platform branch 2 times, most recently from 9cbe47d to e615e0f Compare September 28, 2024 15:08
@talregev
Copy link
Contributor Author

@nibanks
I can work on the second issue with working ci. If you approve my other PR: #4575 then the ci will work automatic for me.

Let me know what do you think about first issue. Is it acceptable from you to change the target name platform to msquic_platform ?

@talregev talregev marked this pull request as draft October 3, 2024 18:37
@MonicaLiu0311
Copy link

Is there any new progress?

@talregev
Copy link
Contributor Author

talregev commented Oct 9, 2024

Is there any new progress?

On my side I need active ci. Because I never contribute to this repo, each change I made, @nibanks need to approve the ci.
I created more simple PR that also needed for msquic. When the other PR will be merge, I will get my active ci here and I can develop.

In conclusion I am waiting for my other PR will be review and merge to continue this PR.
#4575

@talregev
Copy link
Contributor Author

talregev commented Oct 14, 2024

Is there any new progress?

On my side I need active ci. Because I never contribute to this repo, each change I made, @nibanks need to approve the ci. I created more simple PR that also needed for msquic. When the other PR will be merge, I will get my active ci here and I can develop.

In conclusion I am waiting for my other PR will be review and merge to continue this PR. #4575

@BillyONeal can you help to promote this PR? It needed to update msh3 v0.7 in vcpkg.
Then it can promote to make msh3 and msquic non experimental in curl:
curl/curl#15048

@nibanks
Copy link
Member

nibanks commented Oct 14, 2024

@talregev there are many failures in the CI. Please have a look.

@talregev
Copy link
Contributor Author

@talregev there are many failures in the CI. Please have a look.

Thank you! I will take a look, and I start develop. Now I have active ci.

@talregev
Copy link
Contributor Author

talregev commented Oct 14, 2024

@nibanks Not sure why I don't have active ci. How I can get one?
Can you approve the ci again? There will be still errors. I didn't fix them, only fix the conflict.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.84%. Comparing base (9587f74) to head (24ac41f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4574      +/-   ##
==========================================
- Coverage   87.22%   86.84%   -0.39%     
==========================================
  Files          56       56              
  Lines       17357    17357              
==========================================
- Hits        15140    15073      -67     
- Misses       2217     2284      +67     

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

@talregev talregev marked this pull request as ready for review October 15, 2024 16:21
@talregev
Copy link
Contributor Author

@nibanks Ready for review.
Thank you for your help with the ci! 🙏🏻

@BillyONeal
Copy link
Member

@BillyONeal can you help to promote this PR?

I'm not involved in msquic sadly. I have my own PRs here :)

@nibanks
Copy link
Member

nibanks commented Oct 15, 2024

@talregev how did you validate this? Is there any way to add some kind of validation here in this PR?

@talregev
Copy link
Contributor Author

talregev commented Oct 16, 2024

@talregev how did you validate this? Is there any way to add some kind of validation here in this PR?

I create this PR to compile msh3 that msquic is external lib.
msh3 v0.7.0 is needed this symbols:
CxPlatGetSelfSignedCert
CxPlatFreeSelfSignedCert

I validate this by modify locally vcpkg msquic port that will take this changes, and modify msh3 port by update it to v0.7.0 and that it will take ls-qpack and msquic as external libs.
I install it, and i see it compile and not complain the above symbols.

My suggestion to validate in this PR is to create a simple test that use the above symbols. (I am not sure the params that needed for these functions).

Can you help me to create such a simple test that just compile with the above symbols?

@talregev
Copy link
Contributor Author

I notice that there is unittest for platform.
My suggestion is to try to compile it as external cmake. this will validate this PR.
external cmake mean cmake that is not connected to msquic project.
I create locally such a cmake, and I find hard to put it in the ci.
Any suggestion how to do it?

@talregev
Copy link
Contributor Author

@nibanks I manage to create an external cmake and connected it to windows ci.
It still not compile and I feel I very close.
Can you take a look?

@talregev
Copy link
Contributor Author

talregev commented Oct 18, 2024

@nibanks I finished the PR. I also created external cmake that validate the change I did.
It compile on windows. I didn't manage to compile it on Sanitize cases, so I skip those.

Can you review my PR?
Thank you.

@talregev talregev force-pushed the TalR/export_platform branch 2 times, most recently from c20bba6 to 28b4e51 Compare October 18, 2024 14:37
@talregev
Copy link
Contributor Author

talregev commented Oct 22, 2024

@nibanks Anything I can do for move forward this PR?
Thank you for your time and effort!

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry it's taken so long. I've just been unsure of how I want to proceed here. I'm super thankful you added a test to validate, but having this separate cmake seems undesirable. What do you think about updating the normal unittest (platform & core) to dynamic link to msquic.dll / libmsquic.so (which is what I understand this to do), instead of adding a separate file?

Copy link
Contributor Author

@talregev talregev Oct 27, 2024

Choose a reason for hiding this comment

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

What do you think about updating the normal unittest (platform & core) to dynamic link to msquic.dll / libmsquic.so (which is what I understand this to do)

This is not what the test doing.
The external cmake is compile the test platform dynamic and static.
The test validate that it can find the symbols in the official exported lib.
I cannot modify the exist test cmake files. The exist test cmake files are connected to the cmake that generate the libs (dynamic or static). When the cmake is generated, there is no libs yet, and that this point for validate the exported symbol, I must find the libs (static or dynamic).
To test external symbol, it must be different cmake file. One file is to create the lib, and other to use them. (consumers).

@talregev
Copy link
Contributor Author

@nibanks
Copy link
Member

nibanks commented Nov 6, 2024

Can you please merge main once more?

@talregev
Copy link
Contributor Author

talregev commented Nov 6, 2024

Can you please merge main once more?

I will try to merge it today.

@talregev
Copy link
Contributor Author

talregev commented Nov 6, 2024

@nibanks I rebase to main.
Please review again. Thank you!

@nibanks nibanks merged commit a275d74 into microsoft:main Nov 7, 2024
480 of 481 checks passed
@talregev talregev deleted the TalR/export_platform branch November 7, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Build external Proposed by non-MSFT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose CxPlat SelfSignedCert APIs
4 participants