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

[vcpkg-ci] Add test port for cmake user projects #19034

Merged
merged 17 commits into from
Nov 22, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Jul 21, 2021

  • What does your PR fix?

    This PR adds a port which will test configuring, building and installing user projects with the vcpkg cmake toolchain with regard to the following areas:

    • Compatibility with CMake 3.4 for x64 windows/osx (Linux can tested when libidn11 is installed.)
    • VCPKG_PREFER_SYSTEM_LIBS effect (on CMAKE_PREFIX_PATH)
    • find_package() success and linking <PKG>_LIBRARIES for selected ports (via dependencies in vcpkg.json).

    Ideally, this port would always rebuild in CI when toolchain files change.

    This PR complements [vcpkg] Fix toolchain compatibility with cmake < 3.15 #18898. Only the changes to scripts/test_ports/cmake_user (+baseline) are new work.

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

    all triplets, but minimum version test only for x64 windows and osx.
    CI baseline: Skipping x64-linux until package libidn11 is provisioned in order to run cmake 3.4.

  • 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?

    Test port, not registered.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 21, 2021

x64-linux does work locally. Also when merging origin/master.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 21, 2021

linux: cmake 3.4 can't find libidn.so.11.
osx: I need to check the extracted directory for the actual location of cmake.
uwp: "CMake Error: Could not create named generator Visual Studio 16 2019." (Newest generator available: Visual Studio 14 2015.)

@NancyLi1013 NancyLi1013 added category:new-port The issue is requesting a new library to be added; consider making a PR! category:infrastructure Pertaining to the CI/Testing infrastrucutre labels Jul 21, 2021
@dg0yt dg0yt force-pushed the cmake-user branch 7 times, most recently from 1595e08 to 3066d11 Compare July 24, 2021 18:49
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 24, 2021

Ready for review, but leaving draft state until #18898 is merged.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 30, 2021

@BillyONeal Maybe you are already interested in looking at this PR. I'm loosing patience with waiting for #18898.

@PhoebeHui PhoebeHui changed the title Add test port for cmake user projects [vcpkg] Add test port for cmake user projects Aug 2, 2021
@dg0yt dg0yt marked this pull request as ready for review August 5, 2021 07:17
@NancyLi1013 NancyLi1013 added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 6, 2021
@BillyONeal
Copy link
Member

BillyONeal commented Aug 7, 2021

I don't believe doing this as a test port is the appropriate thing, perhaps an e2e test ( https://github.com/microsoft/vcpkg-tool/tree/main/azure-pipelines/end-to-end-tests-dir )? Ports are supposed to always get the version of cmake vcpkg provides (or later), not old ones.

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 7, 2021

Ports are supposed to always get the version of cmake vcpkg provides (or later), not old ones.

This is clear and misses the point of this PR. This PR isn't to test building a port with an older version: It is to simulate a user project environment.

This "port" is just a hook into the automated testing triggered by relevant changes to scripts and ports. For now, it triggers with changes to selected popular ports (catching incompatible changes to wrappers). It should also trigger from changes to vcpkg.cmake scripts if that were part of the port ABI but I don't think this is the case.

It might be related to e2e tests by its "nature", but IIUC these tests are triggered from changing the tool. This doesn't seem the adequate trigger for testing cmake compatibility bugs. The cmake 3.1 (actual 3.4) promise is very likely to be broken by changes to toolchain scripts and wrappers (CMake code), not by changes to the tools (C++ code).

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 18, 2021

While reviewing my open PRs which need work I stumbled over the merge conflict for #19120. I realized that the changes already merged to master made it incompatible with cmake 3.4. (Key problem: no FindIconv module before CMake 3.11.) So I added this as a proof of concept that this port serves its purpose.
Note that the issue is entirely detected when CMake runs for the user project. It doesn't need to use a single symbol from the ports it depends on.
I could probably continue, develop a fix and test it here...

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 5, 2021

@BillyONeal Can you please give directions how to move this forward? Your last post indicated a misunderstanding of what this port is adding to CI, and I tried to rectify this.

@dg0yt dg0yt changed the title [vcpkg] Add test port for cmake user projects [vcpkg-ci] Add test port for cmake user projects Sep 5, 2021
@BillyONeal
Copy link
Member

It might be related to e2e tests by its "nature", but IIUC these tests are triggered from changing the tool. This doesn't seem the adequate trigger for testing cmake compatibility bugs. The cmake 3.1 (actual 3.4) promise is very likely to be broken by changes to toolchain scripts and wrappers (CMake code), not by changes to the tools (C++ code).

The bits in the "scripts" tree are logically part of the tool (and the scripts tree will be moving there too very soon). I wasn't trying to say that this tests vcpkg.exe, only that I believe the tool repo is the right place for tests of this form. Running a test like this as part of the normal "validate ports" pass here is likely to create confusion.

@BillyONeal Can you please give directions how to move this forward? Your last post indicated a misunderstanding of what this port is adding to CI, and I tried to rectify this.

I don't know if we're waiting on anything from you specifically right now.

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 6, 2021

I wasn't trying to say that this tests vcpkg.exe, only that I believe the tool repo is the right place for tests of this form. Running a test like this as part of the normal "validate ports" pass here is likely to create confusion.

IMO the test is for validating changed ports so I'm actually confused by the suggestion to move it to e2e tests. Perhaps it might be a post-build check instead. In any case, it needs to run before merging changes to the vcpkg repo when someone modifies a port.
IIUC these are all options:

Alternative Trigger Scope Control
A) Post-build check Port build Every port, every user vcpkg policy / portfile.cmake
B) Test port Change to ports vcpkg CI dependencies / vcpkg.json
C) E2e test Change to tool vcpkg tool CI

I admit that the highlighting of errors in AZP/Github could be more explicit than just reporting a regression for port cmake-user, in order to reduce confusion.

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 9, 2021

Okay, in this test port are tests looking at other ports (feature find-package in connection with feature cmake-3-4), and tests looking only at the toolchain (VCPKG_PREFER_SYSTEM_LIBS, add_library, add_executable, install). The second group is bonus and might be moved out - but still, it needs to be tested with the cmake version supported for users (feature cmake-3-4).

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 23, 2021

Need to merge master to resolve the conflicts.

Done (trivial).

@NancyLi1013
Copy link
Contributor

Seems all request changes have been applied. Could you please help take a look again if this is ready to merge now? @BillyONeal

@NancyLi1013
Copy link
Contributor

@BillyONeal and @ras0219-msft Could you please help review this PR again? Since all requests seem to be applied.

@NancyLi1013 NancyLi1013 added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 29, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 2, 2021

@BillyONeal CI isn't uploading artifacts for osx, also not in microsoft.vcpkg.ci:

Error: curl failed to put file to https://vcpkgassetcache.blob.core.windows.net/assets/c6969b170e0e8a22e520ccbab137c9f7c509ff6dedd4732c4c318b85e52f0355.zip?*** SECRET *** with exit code '0' and http code '403'

@BillyONeal
Copy link
Member

@BillyONeal CI isn't uploading artifacts for osx, also not in microsoft.vcpkg.ci:

Yeah the credentials expired Friday evening :(. #20512 is the fix.

@strega-nil-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 13, 2021

After the suggestions are applied and Robert's concern(s) are fixed we will merge this.

Ping

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 19, 2021

All checks green again.

@NancyLi1013
Copy link
Contributor

@vicroms, @BillyONeal Could you please help review and merge this PR?

Copy link
Contributor

@ras0219 ras0219 left a comment

Choose a reason for hiding this comment

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

This generally looks good and should function, however I note there is one significant drawback:

By testing every library as a single node, we require restoring binary packages and performing a configure for every library when any library changes. Before this can scale out to 100s of libraries, we will need to change the approach to produce individual test ports per package.

As a suggestion for how this could be done: the comment fields for CMake usage information could be distributed into the producing port. During CI, a script scans for these fields and generates test ports for each cmake usage into an overlay folder. Then, we rely on the normal binary caching / ci procedures to efficiently manage which tests to run.

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 25, 2021

While this port is indeed testing libraries "as a single node", it probably won't depend on more than a limited set of ports soon.

But it can be exploited locally for pre-PR test on other ports. When tests are scripted in CI only, such exploitation is much harder. A future replacement must be implemented in vcpkg.

@PhoebeHui PhoebeHui assigned JackBoosY and unassigned NancyLi1013 Nov 22, 2021
@BillyONeal BillyONeal merged commit d3092a9 into microsoft:master Nov 22, 2021
@dg0yt dg0yt deleted the cmake-user branch November 23, 2021 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:infrastructure Pertaining to the CI/Testing infrastrucutre category:new-port The issue is requesting a new library to be added; consider making a PR! requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants