Skip to content

ci: Move Windows MSVC build from AppVeyor to Cirrus #21551

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
Merged
merged 2 commits into from
Sep 7, 2021
Merged

ci: Move Windows MSVC build from AppVeyor to Cirrus #21551

merged 2 commits into from
Sep 7, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 30, 2021 •
edited
Loading

This PR:

  • gets rid off unreliable AppVeyor CI
  • places all CI configs in one place
  • allows to enable functional tests in the future (using a persistent worker)
no populated vcpkg cache populated vcpkg cache
Screenshot from 2021-09-02 15-47-04 Screenshot from 2021-09-02 14-06-26

Currently, AppVeyor builds take about 44..48 minutes.

@fanquake fanquake added the Tests label Mar 30, 2021
@hebasto
Copy link
Member Author

hebasto commented Mar 30, 2021

The warmed up vcpkg cache reduces build time by about 40 minutes.

@maflcko
Copy link
Member

maflcko commented Mar 30, 2021

Still takes longer than the appveyor 38 minutes right now?

@hebasto
Copy link
Member Author

hebasto commented Mar 30, 2021 •
edited
Loading

Still takes longer than the appveyor 38 minutes right now?

Right.

The time is spent on installing msbuild tools, that are preinstalled on AppVeyor.

@DrahtBot DrahtBot mentioned this pull request Apr 6, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 7, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22899 (ci: Build and cache static Qt instead of downloading a pre-built one by hebasto)
  • #22890 (doc: Replace a link to Qt precompiled binaries with compile instructions by hebasto)
  • #20744 (Use std::filesystem. Remove Boost Filesystem & System by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@hebasto
Copy link
Member Author

hebasto commented Apr 11, 2021

@MarcoFalke

Still takes longer than the appveyor 38 minutes right now?

Does it make sense to move this task into the DrahtBot infrastructure? Or just close it?

@DrahtBot
Copy link
Contributor

I don't run Windows and macOS, so I can't run those native tasks

@hebasto hebasto closed this Apr 11, 2021
@hebasto hebasto reopened this Apr 11, 2021
@hebasto
Copy link
Member Author

hebasto commented Apr 11, 2021

Still takes longer than the appveyor 38 minutes right now?

It takes 27 minutes now.

The time is spent on installing msbuild tools, that are preinstalled on AppVeyor.

Fixed by using visualstudio2019 image.

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 20, 2021
Merge bitcoin#21733: build: Re-add command to install vcpkg
30a86bb 
de17d24 Re-add command to install vcpkg (dplusplus1024)

Pull request description:

  `vcpkg integrate install` must be executed so that msbuild will automatically install external dependencies.

  It was removed in bitcoin@712f95d

  It was originally added in bitcoin@7644567

  <!--
  *** Please remove the following help text before submitting: ***

  Pull requests without a rationale and clear improvement may be closed
  immediately.

  GUI-related pull requests should be opened against
  https://github.com/bitcoin-core/gui
  first. See CONTRIBUTING.md
  -->

  <!--
  Please provide clear motivation for your patch and explain how it improves
  Bitcoin Core user experience or Bitcoin Core developer experience
  significantly:

  * Any test improvements or new tests that improve coverage are always welcome.
  * All other changes should have accompanying unit tests (see `src/test/`) or
    functional tests (see `test/`). Contributors should note which tests cover
    modified code. If no tests exist for a region of modified code, new tests
    should accompany the change.
  * Bug fixes are most welcome when they come with steps to reproduce or an
    explanation of the potential issue as well as reasoning for the way the bug
    was fixed.
  * Features are welcome, but might be rejected due to design or scope issues.
    If a feature is based on a lot of dependencies, contributors should first
    consider building the system outside of Bitcoin Core, if possible.
  * Refactoring changes are only accepted if they are required for a feature or
    bug fix or otherwise improve developer experience significantly. For example,
    most "code style" refactoring changes require a thorough explanation why they
    are useful, what downsides they have and why they *significantly* improve
    developer experience or avoid serious programming bugs. Note that code style
    is often a subjective matter. Unless they are explicitly mentioned to be
    preferred in the [developer notes](/doc/developer-notes.md), stylistic code
    changes are usually rejected.
  -->

  <!--
  Bitcoin Core has a thorough review process and even the most trivial change
  needs to pass a lot of eyes and requires non-zero or even substantial time
  effort to review. There is a huge lack of active reviewers on the project, so
  patches often sit for a long time.
  -->

ACKs for top commit:
  sipsorcery:
    ACK de17d24.
  hebasto:
    ACK de17d24, I use the same in bitcoin#21551.

Tree-SHA512: b32428e450cf76297a0e78b5ffb0933ddcfcce010dbd6f5b87d434bf9c437258edb5b817161f6255d5d60523f79f8513a1fe3e92ba62e3fd3786ab042aaae849
@maflcko maflcko mentioned this pull request Apr 23, 2021
Closed
@fanquake
Copy link
Member

fanquake commented Sep 2, 2021

What is the status of this? Is the intention to replace Appveyor? If not, do we need two Windows builds?

An empty PR description is not really useful.

@maflcko
Copy link
Member

maflcko commented Sep 2, 2021

I started this because Cirrus CI seems the most reliable CI provider, so having all configs on them seemed preferable.

I guess the status here is: Needs review?

@maflcko
Copy link
Member

maflcko commented Sep 2, 2021

(Also needs appveyor removed)

@hebasto hebasto marked this pull request as draft September 2, 2021 09:59
@hebasto hebasto changed the title [PoC] ci: Add Windows task to Cirrus CI ci: Move Windows MSVC build from AppVeyor to Cirrus Sep 2, 2021
@hebasto hebasto marked this pull request as ready for review September 2, 2021 12:15
@hebasto
Copy link
Member Author

hebasto commented Sep 2, 2021

@sipsorcery

It seems Qt5.12.11_x64_static_vs2019_16101.zip is not compatible with the msbuild in the cirrusci/windowsservercore:visualstudio2019 image:

LINK : fatal error C1900: Il mismatch between 'P1' version '20210202' and 'P2' version '20210113' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\bitcoin-qt\bitcoin-qt.vcxproj]
LINK : fatal error LNK1257: code generation failed [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\bitcoin-qt\bitcoin-qt.vcxproj]
LINK : fatal error LNK1327: failure during running link.exe [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\bitcoin-qt\bitcoin-qt.vcxproj]

So, I switched back to Qt5.12.10_x64_static_vs2019_1694.zip for now.

May I ask you to build Qt5.12.11_x64_static_vs2019_1694.zip -- the recent Qt 5.12.11 with an older msbuild?

@sipsorcery
Copy link
Contributor

May I ask you to build Qt5.12.11_x64_static_vs2019_1694.zip -- the recent Qt 5.12.11 with an older msbuild?

Sure, any idea where I can find the software versions installed on the cirrusci image you're using?

@hebasto
Copy link
Member Author

hebasto commented Sep 2, 2021

May I ask you to build Qt5.12.11_x64_static_vs2019_1694.zip -- the recent Qt 5.12.11 with an older msbuild?

Sure, any idea where I can find the software versions installed on the cirrusci image you're using?

https://github.com/cirruslabs/docker-images-windows

@hebasto
Copy link
Member Author

hebasto commented Sep 2, 2021 •
edited
Loading

@sipsorcery

The msbuild -version output within the cirrusci/windowsservercore:visualstudio2019 Cirrus image:

Microsoft (R) Build Engine version 16.9.0+5e4b48a27 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.
16.9.0.16703

@sipsorcery
Copy link
Contributor

The msvc -version output within the cirrusci/windowsservercore:visualstudio2019 Cirrus image:

Microsoft (R) Build Engine version 16.9.0+5e4b48a27 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.
16.9.0.16703

Thanks for doing that. I was trying to track it down via the docker files. I already have that version of msbuild installed so will attempt the Qt build tonight (it kills my machine so overnight job).

@hebasto hebasto force-pushed the 210330-cirrus branch 2 times, most recently from 048eee7 to 310bf9a Compare September 2, 2021 15:15
@sipsorcery
Copy link
Contributor

ACK ca9b9b9.

Makes sense to consolidate CI platforms.

Tested that latest msbuild version 16.11.0.36601 still works with the Qt5.12.11_x64_static_vs2019_160900 binaries.

One potential pitfall will be the vcpkg caching. We had some issues with that in the past, namely whan a PR added or changed a dependency and the vcpkg couldn't deal with it. Since then the vcpkg manifest and binary caching features have been added. They may solve this problem.

@hebasto hebasto added the Windows label Sep 4, 2021
@DrahtBot DrahtBot mentioned this pull request Sep 5, 2021
Merged
@fanquake
Copy link
Member

fanquake commented Sep 6, 2021

ACK after rebase

@hebasto
Copy link
Member Author

hebasto commented Sep 6, 2021

Rebased ca9b9b9 -> da8810e (pr21551.06 -> pr21551.07) due to the conflict with #22861.

@hebasto hebasto mentioned this pull request Sep 6, 2021
Merged
maflcko
maflcko reviewed Sep 6, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Missing BASE_TEMPLATE to merge the latest branch?

.cirrus.yml Outdated
task:
name: "Win64 [unit tests, no functional tests] [msvc]"
windows_container:
cpu: 8
Copy link
Member

Choose a reason for hiding this comment

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

why 8 when the average usage is 2?

Copy link
Member Author

@hebasto hebasto Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

With cpu: 4:

Screenshot from 2021-09-07 06-45-03

And the total task time with a populated vcpkg cache:

Screenshot from 2021-09-07 06-46-09

@hebasto hebasto marked this pull request as draft September 6, 2021 14:51
@hebasto hebasto force-pushed the 210330-cirrus branch 2 times, most recently from ef88865 to 8dacf46 Compare September 6, 2021 15:02
@hebasto hebasto marked this pull request as ready for review September 7, 2021 03:21
@hebasto
Copy link
Member Author

hebasto commented Sep 7, 2021

Updated da8810e -> 97292b1 (pr21551.07 -> pr21551.08, diff):

@maflcko
Copy link
Member

maflcko commented Sep 7, 2021

ack

@hebasto
Copy link
Member Author

hebasto commented Sep 7, 2021

@sipsorcery

Another (final?) review? 😄

@sipsorcery
Copy link
Contributor

re-ACK 97292b1.

@maflcko maflcko merged commit e7c6ed6 into bitcoin:master Sep 7, 2021
@hebasto hebasto deleted the 210330-cirrus branch September 7, 2021 09:30
maflcko
maflcko reviewed Sep 7, 2021
View reviewed changes
@hebasto hebasto mentioned this pull request Sep 7, 2021
Merged
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Sep 7, 2021
Merge bitcoin/bitcoin#22909: ci: Add missed comments and test_bitcoin…
e0ca5d0 
….exe command line option

64015eb ci: Add missed comments and test_bitcoin.exe command line option (Hennadii Stepanov)

Pull request description:

  This PR is a #21551 follow up, and it:
  - adds missed comments, see bitcoin/bitcoin#21551 (comment)
  - restores missed `-l test_suite` command line option for `test_bitcoin.exe`, see bitcoin/bitcoin#21551 (comment)

ACKs for top commit:
  MarcoFalke:
    cr ACK 64015eb

Tree-SHA512: ad1c91544da39a94f033bc55ae5fdaf5774475702edd026635389e68d20e2608cb599dd51f3c1412e0287beef073352e48d9ec005c94df38cfe4fe2d21a94fe3
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 7, 2021
Merge bitcoin#21551: ci: Move Windows MSVC build from AppVeyor to Cirrus
b0c6def
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 7, 2021
Merge bitcoin#22909: ci: Add missed comments and test_bitcoin.exe com…
e7a3455 
…mand line option

64015eb ci: Add missed comments and test_bitcoin.exe command line option (Hennadii Stepanov)

Pull request description:

  This PR is a bitcoin#21551 follow up, and it:
  - adds missed comments, see bitcoin#21551 (comment)
  - restores missed `-l test_suite` command line option for `test_bitcoin.exe`, see bitcoin#21551 (comment)

ACKs for top commit:
  MarcoFalke:
    cr ACK 64015eb

Tree-SHA512: ad1c91544da39a94f033bc55ae5fdaf5774475702edd026635389e68d20e2608cb599dd51f3c1412e0287beef073352e48d9ec005c94df38cfe4fe2d21a94fe3
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 7, 2021
Merge bitcoin#21551: ci: Move Windows MSVC build from AppVeyor to Cirrus
7453000
@ManuNeuro ManuNeuro mentioned this pull request Apr 1, 2022
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @maflcko @DrahtBot @fanquake @sipsorcery