-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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_fixup_pkgconfig] Remove required in first find_program call for pkg-config #12569
Conversation
@Neumann-A there is an error in the paths you are looking into, also |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@Neumann-A if there is not a special reason to keep it, I'd really remove the Cellar folder from the search path. It will become old in a couple of weeks at maximum (we cannot and should not chase it) |
The x64_windows failure is due to a download error and should not block this PR. |
@ras0219 I'll rerun the failed test after x64-osx test finish. |
itk:x64-osx
Your PR #11208 should fix this issue, please add |
@strega-nil, could you help merge this PR firstly? |
Thank you @JackBoosY, @PhoebeHui, and @strega-nil! Looking forward to this being fixed as our CI chain has been down since this started, so test results are also down, and that's a pre-req for merging code. Talk about dominoes eh? Thank you again! |
The ripple effect from this has affected many projects, as
Once its fixed, hopefully some test cases can be added as hard gates in your CI chain to catch this going forward. |
@kcgen thank you for being invested in vcpkg; hopefully, this fixes this issue. I'm not sure how to test this well, since it was caused by an update to CMake; we should look into that. |
Thank you @strega-nil ! Yes; I wonder if testing the installation of some known-stable packages that rely on cmake would catch it. For example, |
there is a PR in which CMake 3.18 is already tested, it should highlight any remaining problem #12612 |
no, vcpkg is still using cmake 3.17 internally if not found on the system. #12612 will upgrade it |
Thanks @cenit; that explains the difficulty in avoiding this exact issue from happening again in the future, like @strega-nil noted.
I suggest two changes to address this:
|
Another way would be to review scripts: this |
Sounds good. But it will also change how Azure DevOps VM image builder works. They need to install cmake and vcpkg together, and ensure cmake version matches. |
For the update of these tools, we should add a test port |
@JackBoosY this is no longer required since script changes rebuilds all affected ports. The CI logic was changed before the PR was merged and it was tested with all affected ports! The problem here is that vcpkg is not testing with the newest cmake version and it thus "slipped" through CI. |
Also, now there is no failure in case pkg-config is not found, on Linux/macOS... |
@cenit is that a problem for you? For me it is theoretically enough if the check is run in the VCPKG CI (or if you do something similar) |
It's just for user experience |
…r pkg-config (microsoft#12569) * Remove required in first find_program call * add usr/local/bin for mac * lets try without cellar and see if osx ci agrees
Due to cmake 3.18 actually respecting it. (Editted from mobile, currently on vacation without wlan access)
closes #12565