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

Add vcpkg ci #13963

Closed
wants to merge 1 commit into from
Closed

Add vcpkg ci #13963

wants to merge 1 commit into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Jun 17, 2024

Add vcpkg ci. I bring you the power of vcpkg. It can build 3rd parties on many os, and can help build curl features and configuration. It can integrate with cmake.
I also add a cache that when it will be other PR and hit the same cache, it will automatic draw the last time compilation. leaving most of the ci time for compile curl and testing.
I also add an upload logs as artifact in case of failing, to see what cause the vpckg to fail.
vcpkg set to the latest, but it can changed. Also most of the time vpckg is stable, but it can have port fails. Usually vcpkg very responsive, especially in the very popular ports (packages) that needed for curl.

I added a tests. tests in osx and windows are fails, not sure why. Maybe I did something in my side. Maybe it can improve curl that way.
Note, I didn't add all the other features for curl because:

  1. curl cmake needed to adapt to be more general (and vcpkg friendly).
  2. libressl is conflict with openssl. by vcpkg rules it cannot compile togther because they conflict in the functions names and more. many ports (packages) that curl needed also compile with them openssl, that mean I cannot add them because of that conflicts.

I fix one of the test. I hope I did it correctly, because on windows it complain that the last line is a dead code that cannot reach.

I will happy to read your feedback and your thoughts.

return error == 0 ? CURLE_OK : TEST_ERR_FAILURE;
if(error == 0) {
return CURLE_OK;
}
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong. What if error is not zero, what does it return then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It going to this line and return:
return (CURLcode)unitfail;
https://github.com/curl/curl/blob/master/tests/unit/curlcheck.h#L110

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think should be the value?
Maybe this line:
https://github.com/curl/curl/blob/master/tests/unit/curlcheck.h#L110
should return TEST_ERR_FAILURE?

Copy link
Contributor Author

@talregev talregev Jun 17, 2024

Choose a reason for hiding this comment

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

I think I know what needed to be done. I understand my error.
I think I should call abort_test fail macro.

@bagder
Copy link
Member

bagder commented Jun 17, 2024

If you want to add directories and files for testing purposes, I think they should be put in tests/ and not in the root dir.

@talregev
Copy link
Contributor Author

talregev commented Jun 17, 2024

If you want to add directories and files for testing purposes, I think they should be put in tests/ and not in the root dir.

I don't mind to add them to tests, but these are configs files that help building vcpkg configurations, and not tests files.
Do you have a 3rd-parties folder?
Maybe inside packages folder?
Maybe it more fit to tests folder.

@talregev talregev force-pushed the TalR/vcpkg branch 6 times, most recently from 039916b to efe4c94 Compare June 17, 2024 22:30
@bagder
Copy link
Member

bagder commented Jun 17, 2024

these are configs files that help building vcpkg configurations

I don't understand. Why do we want files added for building vcpkg configurations? We typically don't host files for building packages because they are usually best authored and maintained by the peeps who build and host those packages.

@talregev
Copy link
Contributor Author

talregev commented Jun 17, 2024

these are configs files that help building vcpkg configurations

I don't understand. Why do we want files added for building vcpkg configurations? We typically don't host files for building packages because they are usually best authored and maintained by the peeps who build and host those packages.

It json files that tell which ports (libs) to build and it using on curl ci, and only the subset that fit for curl and by test or the configuration that needed for curl specific. This is the reason it add here to curl, so I moved them to tests as you suggested.

@talregev talregev force-pushed the TalR/vcpkg branch 2 times, most recently from 8c69618 to d82a125 Compare June 17, 2024 22:49
@dfandrich
Copy link
Contributor

Files needed for packaging curl aren't kept in the curl source repo; the files in packages/ are configs needed for even basic builds on a few platforms. I don't think vcpkg files belong here any more than Debian control files or RPM spec files, or a hundred similar config files. Making sure curl remains able to be packaged by vcpkg using CI jobs is something vcpkg people should be doing, but IMHO it's out of scope of the curl project.

@talregev
Copy link
Contributor Author

talregev commented Jun 18, 2024

I am not package curl via vcpkg.
I am building the libraries that needed for curl compilation via vcpkg in the ci. similar like curl do in the ci sudo apt install libssl-dev
Then curl cmake is building regular like you do cmake building in the ci.
I am also using the cmake install command that on linux do make install like curl do in the ci.

This purpose of the ci is to build the latest libraries needed curl to test curl building with them, then test curl with the libraries with curl ci test.
This is also testing different cmake build options, and test curl cmake itself.

@dfandrich
Copy link
Contributor

Ok, that makes a bit more sense. But what do these builds add that the existing ones don't already do, that get their dependencies with means other than vcpkg? Just using vcpkg doesn't add any value that I can see.

@vszakats
Copy link
Member

vszakats commented Jun 18, 2024

I see the value of vcpkg for MSVC jobs to test with freely chosen dependencies. Serving as the MSVC equivalent of brew and apt. However for macOS and Linux, existing tools work with no issues, what does vcpkg bring there?

As for MSVC, what do you think of integrating vcpkg into the existing MSVC workflow in windows.yml? (rather than rolling their separate .yml, with potentially redundant CI jobs)

Can we use vcpkg without a tree of config files and empty directory committed to the repo? Does vcpkg support requesting dependencies without those files, from the command-line? It'd be a big plus if CI logic would remain self-contained within their CI .yaml files.

Why is it necessary to modify a unit test result?

@bagder
Copy link
Member

bagder commented Jun 18, 2024

Serving as the MSVC equivalent of brew and apt

We use brew and apt to install a lot of dependencies for CI jobs. That's their purpose. I don't see that this newly built vcpkg in these CI jobs is actually used to install dependencies for builds. So why do we build it?

@vszakats
Copy link
Member

vszakats commented Jun 18, 2024

What I meant is for MSVC jobs, there is no mechanism to install dependencies, like we're used to with brew and apt (etc). For these MSVC jobs, vcpkg is the package manager. Without it, the only way to use dependencies is to either have them installed by default, or install them manually, which is very painful, with different methods for each dependency.

MSYS2 solves this for mingw-w64. Cygwin also has it solved.

So basically all platforms have a package manager of some kind, except MSVC, and vcpkg seems to plug that hole.

For other targets, I don't see the benefit of vcpkg, in curl's context.

@talregev
Copy link
Contributor Author

talregev commented Jun 18, 2024

I see the value of vcpkg for MSVC jobs to test with freely chosen dependencies. Serving as the MSVC equivalent of brew and apt. However for macOS and Linux, existing tools work with no issues, what does vcpkg bring there?

The main different from apt (I am not sure about brew) that vcpkg bring you the latest library version that exist in vcpkg that usually the latest version library that exist. especially on the libraries that are very popular that curl are using.
For example in openssl. openssl on ubuntu is 3.0.x, and on vcpkg is bring you 3.3.1 (couple days after they release it).
For osx there some tests that was failed, I think it due the cases is not check in the osx ci, and now you can add easily cases and improve osx.

As for MSVC, what do you think of integrating vcpkg into the existing MSVC workflow in windows.yml? (rather than rolling their separate .yml, with potentially redundant CI jobs)

It can be done to integrate vcpkg to windows.yml. but You have working ci, and before you commit to vcpkg, you should check it on sevral PRs and then can integrate it to one file after you see it working as you expected. Also this is work in progress, because you should also adapt cmake that will find all other libraries from vcpkg that curl needed that currently is not the case. for example zlib. So it better to have different ci that easily change / remove as you want. and over time to integrate one to another.

Can we use vcpkg without a tree of config files and empty directory committed to the repo? Does vcpkg support requesting dependencies without those files, from the command-line? It'd be a big plus if CI logic would remain self-contained within their CI .yaml files.

Yes I did. I refactor the ci files and remove the vcpkg folder entirely.

Why is it necessary to modify a unit test result?

On windows on openssl build option, it create this compilation warning / error:

D:\a\curl\curl\tests\unit\unit2604.c(111) : error C2220: the following warning is treated as an error
D:\a\curl\curl\tests\unit\unit2604.c(111) : warning C4702: unreachable code

So I change the test file that it will be without unreachable code. I would like to have a feedback if I did the fix correctly, or how I can fix correctly that solve the unreachable code problem.

Thank you on your feedback.

Other tests on osx and windows are failing too, and I would like help how to fix them.
Also curl compilation on osx and windows failed with perl command. I think it try to build documentation and failed. I am not sure why. How I can fix it, and if not, I can disable that curl will not build documentation on these builds.

@talregev talregev force-pushed the TalR/vcpkg branch 3 times, most recently from 9119af4 to bb22d5e Compare June 18, 2024 20:24
@vszakats
Copy link
Member

I see the value of vcpkg for MSVC jobs to test with freely chosen dependencies. Serving as the MSVC equivalent of brew and apt. However for macOS and Linux, existing tools work with no issues, what does vcpkg bring there?

The main different from apt (I am not sure about brew) that vcpkg bring you the latest library version that exist in vcpkg that usually the latest version library that exist. especially on the libraries that are very popular that curl are using. For example in openssl. openssl on ubuntu is 3.0.x, and on vcpkg is bring you 3.3.1 (couple days after they release it). For osx there some tests that was failed, I think it due the cases is not check in the osx ci, and now you can add easily cases and improve osx.

For apt we can have this effect by using debian:unstable or a rolling distro of choice. For specific deps we solve this by building the them from source. This is mostly painless on Linux; on the plus side it's standard, everyone understands it. I think it's also a plus to avoid large dependencies like vcpkg unless unavoidable. brew offers rolling releases as standard.

As for MSVC, what do you think of integrating vcpkg into the existing MSVC workflow in windows.yml? (rather than rolling their separate .yml, with potentially redundant CI jobs)

It can be done to integrate vcpkg to windows.yml. but You have working ci, and before you commit to vcpkg, you should check it on sevral PRs and then can integrate it to one file after you see it working as you expected. Also this is work in progress, because you should also adapt cmake that will find all other libraries from vcpkg that curl needed that currently is not the case. for example zlib. So it better to have different ci that easily change / remove as you want. and over time to integrate one to another.

I'd be happy with integrating as part of this PR. Not least, CI tests are CPU/time expensive and add flakyness.

What do you think of making the two no-SSL (= not so useful) MSVC jobs in windows.yml more useful by enabling OpenSSL / LibreSSL respectively? with your libssh2 settings. Seems like a perfect fit.

Can we use vcpkg without a tree of config files and empty directory committed to the repo? Does vcpkg support requesting dependencies without those files, from the command-line? It'd be a big plus if CI logic would remain self-contained within their CI .yaml files.

Yes I did. I refactor the ci files and remove the vcpkg folder entirely.

Looking good, thanks!

On windows on openssl build option, it create this compilation warning / error:

D:\a\curl\curl\tests\unit\unit2604.c(111) : error C2220: the following warning is treated as an error
D:\a\curl\curl\tests\unit\unit2604.c(111) : warning C4702: unreachable code

So I change the test file that it will be without unreachable code. I would like to have a feedback if I did the fix correctly, or how I can fix correctly that solve the unreachable code problem.

Fair enough. Would you mind adding this to the commit/PR message?

@talregev
Copy link
Contributor Author

talregev commented Jun 18, 2024

Fair enough. Would you mind adding this to the commit/PR message?

I think I will make a different PR for test fix.

Do you want me to delete the ci for linux and osx?
Will you attempted to fix the tests failing on osx vcpkg ci? (Even if you don't want the ci itself).

I am not sure I want to refactor windows.yml. It more time for me. I can make vcpkg-windows.yml more usefull ci.

@bagder
Copy link
Member

bagder commented Jun 19, 2024

The Windows job fails test 1516 because the shell used there converts the /path/1516 string given as an argument to the test code into c:/../path/1516 which the test program does not deal with.

@talregev
Copy link
Contributor Author

talregev commented Jun 19, 2024

@bagder Also interesting test that can fix easily:

test 1013...[Compare curl --version with curl-config --protocols]
Mismatch in protocols lists:
curl: dict file ftp ftps gopher gophers http https imap i imaps ipfs ipns ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
curl-config: dict file ftp ftps gopher gophers http https imap imaps ipfs ipns ldap ldaps mqtt pop3 pop3s rtsp scp sftp smtp smtps telnet tftp
1013: postcheck FAILED

On vcpkg-osx.yml secure transport option

What is the missing smb smbs protocols?
How I can add them in cmake build for curl-config?
Is it CURL_USE_SECTRANSP is connected here?

@vszakats
Copy link
Member

Fair enough. Would you mind adding this to the commit/PR message?

I think I will make a different PR for test fix.

Do you want me to delete the ci for linux and osx? Will you attempted to fix the tests failing on osx vcpkg ci? (Even if you don't want the ci itself).

I am not sure I want to refactor windows.yml. It more time for me. I can make vcpkg-windows.yml more usefull ci.

I prefer your #13979 over this PR. Left a bunch of reviews with mostly small things.

@bagder
Copy link
Member

bagder commented Jun 30, 2024

Does this PR still have merit after #13979 was merged?

@talregev
Copy link
Contributor Author

Does this PR still have merit after #13979 was merged?

I left this one open because there is a bug in osx that the tests 1013 and 1014 failed.

vszakats added a commit to vszakats/curl that referenced this pull request Jun 30, 2024
NTLM and SMB were missing from the feature list for build using
SecureTransport.

Follow-up to 76a9c3c curl#3619

Reported-by: Tal Regev
Bug: curl#13963 (comment)
Closes #xxxxx
@vszakats
Copy link
Member

@talregev Thanks for the heads up, I've opened #14065 for the smb/smbs protocol-, and NTLM in the feature-list fallouts.

@vszakats
Copy link
Member

(I think it's safe to close this now.)

@talregev talregev closed this Jul 1, 2024
@talregev
Copy link
Contributor Author

talregev commented Jul 1, 2024

Thank you for fixing the tests!
you should also add a tests case that check these bugs.

@talregev talregev deleted the TalR/vcpkg branch July 1, 2024 04:18
vszakats added a commit that referenced this pull request Jul 1, 2024
NTLM was missing from the features list, and SMB/SMBS from
the protocols list in SecureTransport builds.

Follow-up to 76a9c3c #3619

Reported-by: Tal Regev
Bug: #13963 (comment)
Closes #14065
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration tests
Development

Successfully merging this pull request may close these issues.

None yet

4 participants