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

[saucer, lockpp] add new port #25111

Merged
merged 13 commits into from
Jun 16, 2022
Merged

[saucer, lockpp] add new port #25111

merged 13 commits into from
Jun 16, 2022

Conversation

Curve
Copy link
Contributor

@Curve Curve commented Jun 7, 2022

Describe the pull request

  • What does your PR fix?

    Adds new port for saucer and lockpp

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    static, non uwp, Yes

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

@ghost
Copy link

ghost commented Jun 7, 2022

CLA assistant check
All CLA requirements met.

@Curve Curve mentioned this pull request Jun 7, 2022
Curve added 2 commits June 7, 2022 15:49
[saucer] add lockpp dependency

beleg
@Curve Curve changed the title [saucer] add new port [saucer, lockpp] add new port Jun 7, 2022
@Cheney-W Cheney-W added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jun 8, 2022
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for saucer have changed but the version was not updated
version: 0.1.0
old SHA: f4101555f119b7d120f7592b2d1ce69d192daa4f
new SHA: 499dd7901ebd67f39c999ba3abc7f312dbf59f25
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@Cheney-W
Copy link
Contributor

Cheney-W commented Jun 8, 2022

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for saucer have changed but the version was not updated
version: 0.1.0
old SHA: f4101555f119b7d120f7592b2d1ce69d192daa4f
new SHA: 499dd7901ebd67f39c999ba3abc7f312dbf59f25
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
No files were updated

@Curve
Copy link
Contributor Author

Curve commented Jun 8, 2022

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for saucer have changed but the version was not updated
version: 0.1.0
old SHA: f4101555f119b7d120f7592b2d1ce69d192daa4f
new SHA: 499dd7901ebd67f39c999ba3abc7f312dbf59f25
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
No files were updated

I did but I must have messed something up, I will fix this later

@Curve
Copy link
Contributor Author

Curve commented Jun 9, 2022

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for saucer have changed but the version was not updated version: 0.1.0 old SHA: f4101555f119b7d120f7592b2d1ce69d192daa4f new SHA: 499dd7901ebd67f39c999ba3abc7f312dbf59f25 Did you remember to update the version or port version? Use --overwrite-version to bypass this check No files were updated

Should be fixed ^^

@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Jun 10, 2022
ports/lockpp/vcpkg.json Outdated Show resolved Hide resolved
ports/saucer/vcpkg.json Outdated Show resolved Hide resolved
@Cheney-W Cheney-W added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jun 10, 2022
@Thomas1664
Copy link
Contributor

Thomas1664 commented Jun 10, 2022

@Curve I noticed another potential issue with your ports: They have commonly used include file names:

saucer:x64-windows-static:/include/annotations.hpp
saucer:x64-windows-static:/include/events/events.hpp
saucer:x64-windows-static:/include/events/events.inl
saucer:x64-windows-static:/include/modules/core.module.hpp
saucer:x64-windows-static:/include/modules/module.hpp
saucer:x64-windows-static:/include/promise/promise.hpp
saucer:x64-windows-static:/include/promise/promise.inl
saucer:x64-windows-static:/include/serializers/json.hpp
saucer:x64-windows-static:/include/serializers/json.inl
saucer:x64-windows-static:/include/serializers/serializer.hpp
saucer:x64-windows-static:/include/serializers/serializer.inl
saucer:x64-windows-static:/include/smartview.hpp
saucer:x64-windows-static:/include/smartview.inl
saucer:x64-windows-static:/include/webview.hpp
saucer:x64-windows-static:/include/window.hpp
lockpp:x64-windows-static:/include/lock.hpp
lockpp:x64-windows-static:/include/lock.inl
lockpp:x64-windows-static:/include/locked.hpp
lockpp:x64-windows-static:/include/locked.inl
lockpp:x64-windows-static:/include/mutex_traits.hpp

Especially saucer:x64-windows-static:/include/events/events.hpp may cause file conflicts with other ports. Because you are the author of upstream, I suggest that you move your include files to a subdirectory, i.e. lockpp or saucer.

ports/saucer/vcpkg.json Outdated Show resolved Hide resolved
@Curve
Copy link
Contributor Author

Curve commented Jun 11, 2022

@Curve I noticed another potential issue with your ports: They have commonly used include file names:

saucer:x64-windows-static:/include/annotations.hpp
saucer:x64-windows-static:/include/events/events.hpp
saucer:x64-windows-static:/include/events/events.inl
saucer:x64-windows-static:/include/modules/core.module.hpp
saucer:x64-windows-static:/include/modules/module.hpp
saucer:x64-windows-static:/include/promise/promise.hpp
saucer:x64-windows-static:/include/promise/promise.inl
saucer:x64-windows-static:/include/serializers/json.hpp
saucer:x64-windows-static:/include/serializers/json.inl
saucer:x64-windows-static:/include/serializers/serializer.hpp
saucer:x64-windows-static:/include/serializers/serializer.inl
saucer:x64-windows-static:/include/smartview.hpp
saucer:x64-windows-static:/include/smartview.inl
saucer:x64-windows-static:/include/webview.hpp
saucer:x64-windows-static:/include/window.hpp
lockpp:x64-windows-static:/include/lock.hpp
lockpp:x64-windows-static:/include/lock.inl
lockpp:x64-windows-static:/include/locked.hpp
lockpp:x64-windows-static:/include/locked.inl
lockpp:x64-windows-static:/include/mutex_traits.hpp

Especially saucer:x64-windows-static:/include/events/events.hpp may cause file conflicts with other ports. Because you are the author of upstream, I suggest that you move your include files to a subdirectory, i.e. lockpp or saucer.

Changed upstream

@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 13, 2022
@Curve Curve requested a review from Cheney-W June 13, 2022 13:40
@ras0219-msft ras0219-msft added the depends:different-pr This PR or Issue depends on a PR which has been filed label Jun 13, 2022
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Once #25217 merges, could you use that as a dependency instead of embedding the NuGet package?

ports/saucer/vcpkg.json Outdated Show resolved Hide resolved
@Cheney-W Cheney-W added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jun 14, 2022
@Curve
Copy link
Contributor Author

Curve commented Jun 14, 2022

Thanks for the PR!

Once #25217 merges, could you use that as a dependency instead of embedding the NuGet package?

Of course

Edit: Oh, merging the suggestion through the GitHub issue obviously doesn't update the Sha, I'll also fix this later

Co-authored-by: Robert Schumacher <[email protected]>
@Curve
Copy link
Contributor Author

Curve commented Jun 14, 2022

@ras0219-msft I fixed the current build with the removed catch2 dependency, let me know when the WebView2 Port gets merged then I'll add it as a dependency

Edit: Also, you may want to let your colleagues from the WebView2 Team know that a port for it is already in progress (MicrosoftEdge/WebView2Feedback#889)

@Cheney-W
Copy link
Contributor

WebView2 has been merged now.

@Cheney-W Cheney-W removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Jun 15, 2022
@Curve
Copy link
Contributor Author

Curve commented Jun 15, 2022

@ras0219-msft I've made the port use the vcpkg webview2 port

@Curve Curve requested a review from ras0219-msft June 15, 2022 15:49
@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 16, 2022
@dan-shaw dan-shaw merged commit ca8bde3 into microsoft:master Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants