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.

Sign up for GitHub

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

Jump to bottom

[libpsl,curl,vcpkg-ci-curl] Update suffix list, fix and test curl #38847

Merged
merged 6 commits into from
Jun 11, 2024
Merged

[libpsl,curl,vcpkg-ci-curl] Update suffix list, fix and test curl #38847

merged 6 commits into from
Jun 11, 2024
+111 −91

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 21, 2024

No description provided.

@talregev
Copy link
Contributor

talregev commented May 21, 2024 •
edited

You can use my fix that it will install the feature in windows static, also it need to modify that it will have pkgconfig for psl feature as you suggested.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 21, 2024

You can use my fix that it will install the feature in windows static, also it need to modify that it will have pkgconfig for psl feature as you suggested.

No conditional patches, thanks.

@talregev
Copy link
Contributor

You can use my fix that it will install the feature in windows static, also it need to modify that it will have pkgconfig for psl feature as you suggested.

No conditional patches, thanks.

How you want to implement your suggestion?

@talregev
Copy link
Contributor

talregev commented May 21, 2024 •
edited

I can close my PR, and you can fix this. Thank you for your work!.
and by the way, curl[psl] do compile on uwp after the fixes.

@talregev talregev mentioned this pull request May 21, 2024
Closed
7 tasks
@dg0yt
Copy link
Contributor Author

dg0yt commented May 21, 2024

As I wrote in review comments: replace after installation. vcpkg_replace_string, found in 100 or 200 ports.

@talregev
Copy link
Contributor

As I wrote in review comments: replace after installation. vcpkg_replace_string, found in 100 or 200 ports.

I look again on your comments:

#38820 (comment)
#38828 (comment)

You didn't write about the implementation. I did what I know. If you would have tell me with vcpkg_replace_string I would done it. I am learning along the way.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 21, 2024

#38820 (comment): "after the install step".

@talregev
Copy link
Contributor

#38820 (comment): "after the install step".

As I say, I didn't understand you.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 21, 2024

Even port curl has the pattern:

vcpkg/ports/curl/portfile.cmake

Lines 134 to 139 in 261dd68

if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/curl/curl.h"
"#ifdef CURL_STATICLIB"
"#if 1"
)
endif()

@talregev
Copy link
Contributor

Even port curl has the pattern:

vcpkg/ports/curl/portfile.cmake

Lines 134 to 139 in 261dd68

if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/curl/curl.h"
"#ifdef CURL_STATICLIB"
"#if 1"
)
endif()

I didn't read all the curl port. As you notice, I also miss the PKGCONFIG per feature. once you told me about that, I learn about it.

talregev
talregev reviewed May 21, 2024
View reviewed changes
scripts/test_ports/vcpkg-ci-curl/vcpkg.json Outdated
"features": [
"psl"
],
"platform": "!android & !uwp"
Copy link
Contributor

@talregev talregev May 21, 2024 •
edited

Choose a reason for hiding this comment

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

Can you try remove !uwp? As I test it, it also compile curl[psl].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. libpsl has !uwp.

And libpsl can only be changed once the test is known to work, not before.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, Thank you for your answer.

talregev
talregev reviewed May 21, 2024
View reviewed changes
ports/curl/portfile.cmake Outdated
@@ -54,7 +54,7 @@ vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
)

set(OPTIONS "")
if("idn2" IN_LIST FEATURES OR ("ldap" IN_LIST FEATURES AND NOT VCPKG_TARGET_IS_WINDOWS))
if(";${FEATURES};" MATCHES ";(idn2|gssapi|psl);" OR ("ldap" IN_LIST FEATURES AND NOT VCPKG_TARGET_IS_WINDOWS))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I add this line to my refactor curl PR? then it will be less changes to this port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code will remain in this PR: This port must enable PKGCONFIG for psl, and there is no reason to ignore gssapi.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to remove the code from this PR. I ask permission to use your code on my PR. when it will be merge to master, you can rebase it back to you.

@dg0yt dg0yt force-pushed the libpsl branch 3 times, most recently from d41c008 to 6072a89 Compare May 22, 2024 08:13
@FrankXie05 FrankXie05 added the category:port-bug The issue is with a library, which is something the port should already support label May 22, 2024
@BillyONeal
Copy link
Member

There are already lots of failures here and this got stuck on the networking change so I just cancelled the remaining runs

@dg0yt dg0yt mentioned this pull request May 24, 2024
Merged
@dg0yt
Copy link
Contributor Author

dg0yt commented May 27, 2024

Should wait for merge of #38921.

@talregev
Copy link
Contributor

talregev commented May 28, 2024 •
edited

@dg0yt Can you cherry pick the fixes for libpsl and curl and put them in other PR without the fixes is needed for vcpkg-ci-curl port. These are important fixes.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 28, 2024

Can you cherry pick the fixes for libpsl and curl and put them in other PR without the fixes is needed for vcpkg-ci-curl port. These are important fixes.

No. These fixes are to be merged with vcpkg-ci-curl test coverage. Because it avoids regressions and poor UX.

@dg0yt dg0yt mentioned this pull request May 30, 2024
Closed
vicroms pushed a commit that referenced this pull request Jun 6, 2024
@dg0yt
[vcpkg-cmake-config] Fix fixup of ) paths (#38921)
233f096 
Cherry-picked from #38847:

Fix processing of `INTERFACE_LINK_LIBS` when it contain `)` inside `"`,
e.g. for
`C:/Program Files (x86)/Windows
Kits/10/Lib/10.0.22621.0/um/x64/User32.Lib`
created by `pkg_check_modules` from `-luser32` as found in icu pc files.

Without the fix, the property is not processed at all, pulling release
libs into debug builds.
dg0yt added 2 commits June 6, 2024 17:24
@dg0yt
[libpsl] Update suffix list
7d8b2c7 
And add a disambiguation to the filename.
@dg0yt
[libpsl] Minor changes
72abf75
@dg0yt dg0yt changed the title [vcpkg-ci-curl] Test feature psl [libpsl,curl,vcpkg-ci-curl] Update suffix list, fix and test curl Jun 7, 2024
@dg0yt dg0yt marked this pull request as ready for review June 8, 2024 06:41
@BillyONeal BillyONeal merged commit 795f2f1 into microsoft:master Jun 11, 2024
17 checks passed
@BillyONeal
Copy link
Member

Thanks for the updates and tests!

@dg0yt dg0yt deleted the libpsl branch June 11, 2024 02:07
@vszakats vszakats mentioned this pull request Jun 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talregev talregev talregev left review comments

Assignees

@FrankXie05 FrankXie05

Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dg0yt @talregev @BillyONeal @FrankXie05