ci: Avoid failing pull requests destory the appveyor cache

MarcoFalke commented at 11:38 am on July 2, 2020: member

ci: Avoid failing pull requests destory the appveyor cache 666686f664

fanquake added the label Tests on Jul 2, 2020

MarcoFalke closed this on Jul 2, 2020

MarcoFalke reopened this on Jul 2, 2020

MarcoFalke commented at 1:17 pm on July 2, 2020: member

@sipsorcery Any thoughts on this?

Also, unrelated, I was wondering how to enable C++17 in appveyor. I.e. how to pass down this option: https://docs.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=vs-2019

sipsorcery commented at 8:46 pm on July 2, 2020: member

@MarcoFalke the appveyor cache is being used to store dependencies not the build results:

0cache:
1- C:\tools\vcpkg\installed -> build_msvc\vcpkg-packages.txt
2- C:\Qt5.9.8_x64_static_vs2019

If the cache is wiped out by setting APPVEYOR_SAVE_CACHE_ON_ERROR: false then the next build after a failure will have to restore all the dependencies. This will more than likely result in the build never being able to complete since restoring dependencies, building Bitcoin Core and running the tests is unlikely to fit within the appveyor time limit (depending on what the time limit currently allocated to Bitcoin Core is). I’d strongly advise against merging this change. After the first failed build it’s likely all subsequent appveyor builds will fail with a timeout.

At one point clcache was used to cache the build results but during my testing I was never convinced it reduced the build time (the project seems to be discontinued now so perhaps I wasn’t the only one with that finding).

For the C++17 build it will need a compiler option set in common.init.vcxproj. I can test and submit a PR for that if you like? I can remove the CLCACHE_SERVER: 1 variable at the same time.

MarcoFalke commented at 9:15 pm on July 2, 2020: member

This will more than likely result in the build never being able to complete

I have concluded the opposite:

The same cache is wiped when build_msvc/vcpkg-packages.txt changes. So if builds never could succeed from scratch, then how did we get the first compilation to finish? Or how could we change packages in the future if the cache never gets invalidated?
If a build times out, why would there be any time to upload the cache? I think APPVEYOR_SAVE_CACHE_ON_ERROR refers to a build or test error, not a timeout.
This morning a pull request failed to build and seemingly corrupted the cache. See the picture and links below.

First failure (pull): https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/33856815 First passing build (The pull that fixed it by removing the cache): https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/33867492

Interestingly the first passing build with the invalidated cache took the same time to build as it takes other builds to run, so maybe the cache is not needed after all?

Screenshot_2020-07-02 Bitcoin Core - AppVeyor

sipsorcery commented at 10:05 pm on July 2, 2020: member

The same cache is wiped when build_msvc/vcpkg-packages.txt changes. So if builds never could succeed from scratch, then how did we get the first compilation to finish? Or how could we change packages in the future if the cache never gets invalidated?

It is/was a juggling act. On my fork I have an appveyor-force-refresh branch (bit out of date now) that uses Exit-AppveyorBuild after the dependencies are installed that exits the job before the build solely to get the dependency cache set. After that run I’d switch back to master and do the build with the dependencies already in the cache.

If a build times out, why would there be any time to upload the cache? I think APPVEYOR_SAVE_CACHE_ON_ERROR refers to a build or test error, not a timeout.

Yes you are right my mistake. As per above I used to have to exit the job early to avoid a timeout in order to get the cache updated.

Interestingly the first passing build with the invalidated cache took the same time to build as it takes other builds to run, so maybe the cache is not needed after all?

From a quick look at the appveyor job history there was a spike of jobs rebuilding the vcpkg dependencies. I suspect, but didn’t verify, that a PR changed the build_msvc\vcpkg-packages.txt file and thereby invalidated the C:\tools\vcpkg\installed cache entry. The subsequent PR most likely had the original packages file causing it to also invalidate the cache.

The following jobs all use the vcpkg cache WITHOUT re-generating:

master.22376 45:21
master.22375 44:19
master.22374 45:27
master.22373 45:51

These jobs DID rebuild the vcpkg dependencies:

master.22370 60:04
master.22369 60:03
master.22368 60:02

So a consistent 15 minute difference if vcpkg dependencies need to be rebuilt. Also the Bitcoin Core appveyor jobs do seem to have more resources than free/personal appveyor accounts. My own appveyor account can’t build + test Bitcoin core in under an hour even with the dependencies in the cache.

But I know where you’re coming from, caches are always a pain precisely because of issues like this. I was obviously a bit melodramatic when I said all builds will fail if this PR is merged. The builds referenced above show they are still passing even when the vcpkg dependencies are rebuilt, they just take 15 minutes longer.

I guess if the cache is causing issues AND the build can still be done under the appveyor time limit it does make sense to remove it. Waiting an hour for a CI job seems like it’s too long but if there’s a queue of jobs an extra 15 minutes is not noticeable. It would mean free/personal appveyor accounts are unlikely to ever be able to use this script successfully but that’s the case now any way.

MarcoFalke commented at 11:08 am on July 3, 2020: member

Oh, I see. I think we got our max timeout raised by staff at some point, which you can also raise in your forked repo.

Lets continue the C++17 discussion in #16684 (comment)

Closing this pull for now.

MarcoFalke closed this on Jul 3, 2020

MarcoFalke deleted the branch on Jul 3, 2020

MarcoFalke restored the branch on Jul 4, 2020

MarcoFalke reopened this on Jul 4, 2020

MarcoFalke commented at 10:52 am on July 4, 2020: member

reopen per #19440 (comment)

MarcoFalke force-pushed on Jul 4, 2020

MarcoFalke commented at 12:39 pm on July 4, 2020: member

Going to merge this to see if it helps with #19440 (comment)

If not, the cache should probably be removed altogether.

sipsorcery commented at 12:40 pm on July 4, 2020: member

ACK 666686f664b47fae4bf5900e8c4acb808ef73063.

DrahtBot commented at 2:57 pm on July 4, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19444 (test: Remove cached directories and associated script blocks from appveyor config by sipsorcery)

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.

MarcoFalke closed this on Jul 4, 2020

MarcoFalke deleted the branch on Jul 4, 2020

MarcoFalke referenced this in commit 8783bcc099 on Jul 4, 2020

DrahtBot locked this on Feb 15, 2022

ci: Avoid failing pull requests destory the appveyor cache #19431

Conflicts