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

appveyor: Use clcache to speed up build #14086

Merged
merged 1 commit into from Sep 4, 2018

Conversation

Projects
None yet
6 participants
@ken2812221
Copy link
Member

commented Aug 27, 2018

https://ci.appveyor.com/project/ken2812221/bitcoin/build/patch-4.407

The build time reduced from 18 mins to 7 mins.

  • clcache is a third-party software, act much like ccache. (Compile-time cache)
  • *.iobj and *.ipdb is a MSVC built-in cache. (Link-time cache)
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2018

No more conflicts as of last run.
@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@ken2812221 Promising numbers - nice work! Any potential drawbacks?

@fanquake fanquake added the Tests label Aug 27, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Any potential drawbacks?

I am not sure if there is any. This is my first time trying this stuff.

Oh, there is one. We cannot use appveyor logger anymore, it's incompatible with clcache.

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Update: It is really hard to see where the error is if it print a lot of warning messages, so I just disable the warnings.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@ken2812221 Actually there are only two warnings (C4244 and C4267) that make the log output unreadable due to the sheer amount of them. Could you try disabling those two warnings specifically instead of disabling all warnings?

I analyzed the warnings:

$ grep ": warning " log.txt | cut -f4- -d" " | cut -f1 -d: | grep ^C | sort | uniq -c
   1 C4018 == signed/unsigned mismatch
   8 C4101 == unreferenced local variable
2056 C4244 == conversion from […] to […], possible loss of data
1482 C4267 == conversion from […] to […], possible loss of data
   1 C4305 == truncation from 'int' to 'bool'
   1 C4312 == conversion from 'int' to 'void *' of greater size
   1 C4334 == result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
   8 C4715 == not all control paths return a value 

The total number of warnings 3558 is reduced to only 20 warnings when excluding C4244 and C4267.

Having warnings enabled adds additional value to the appveyor integration by giving access to the MSVC warnings (which are good!) also to developers who don't use Windows. I know I enjoy having access to them :-)

@ken2812221 ken2812221 force-pushed the ken2812221:patch-4 branch Aug 28, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:patch-4 branch 5 times, most recently Aug 28, 2018

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@ken2812221 Excellent!

utACK 637f88c7e3142dde123b40982bb6418e640c3e42

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@ken2812221 What about disabling C4715: '[…]': not all control paths return a value too? We intentionally assert(false); unconditionally (in default:) in eight places which we don't want to change. This triggers C4715. That warning is not actionable in our case so I think disabling it is a net win.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-4 branch Aug 28, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:patch-4 branch to 5151f5e Aug 28, 2018

MarcoFalke added a commit that referenced this pull request Aug 29, 2018

Merge #14093: tests: Fix accidental trunction from int to bool
1cc5897 tests: Fix accidental trunction from int to bool (practicalswift)

Pull request description:

  Fix accidental trunction from `int` to `bool`.

  Context: #14086 (comment)

Tree-SHA512: 72d209f892e580afa9c295174c206ea5ba764ff9e03613cd9bc57fd0d7118e895ee44d96db90930a29c0b4de7f51dc00101a1b32ba6b46576d34e089ff5482ba
@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2018

@MarcoFalke Mind review this?
@practicalswift Disabled C4715.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

Concept ACK, but I don't know enough of appveyor and clcache to actually review this.

Maybe @sipsorcery ?

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2018

@ken2812221 are you currently able to get the build based on the appveyor.yml file to work? My attempts are failing with:

LINK : fatal error C1047: The object or library file 'C:\Tools\vcpkg\installed\x64-windows-static\lib\libzmq-mt-s-4_3_1.lib' was created with an older compiler than other objects; rebuild old objects and libraries [C:\projects\bitcoin-72c17\build_msvc\bitcoind\bitcoind.vcxproj]
LINK : fatal error LNK1257: code generation failed [C:\projects\bitcoin-72c17\build_msvc\bitcoind\bitcoind.vcxproj]

So far I've been unsuccessful in getting the libzmq dependency to update (my mingw32 build is also failing because of libzmq so something has changed there recently). I have managed to get my custom AppVeyor Bitcoin build that doesn't use appveyor.yml to work so I'm pretty sure this failure is due to the libzmq dependency needing to be updated.

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2018

@sipsorcery You can just clear the cache by this. Every thing should work fine.

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2018

tACK

Build time is reduced from ~22mins to ~11mins.

@ken2812221 do you think it would be worth adding artifacts to the AppVeyor? It would be nice to be able to use the results of the build even if it's just for sanity testing.

artifacts:
- path: build_msvc\%PLATFORM%\%CONFIGURATION%\*.exe
  name: Executables
@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2018

do you think it would be worth adding artifacts to the AppVeyor?

There is a previous discussion in #13855. Many devs thought that uploading executables is not a good idea.

@ken2812221 ken2812221 force-pushed the ken2812221:patch-4 branch 2 times, most recently Sep 3, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:patch-4 branch to e4a79b4 Sep 3, 2018

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Sep 3, 2018

Increase signal to noise in appveyor build output by reducing the MSV…
…C warning count from 12 to 4 (12 is assuming the changes in bitcoin#14086 are also implemented).

This makes it easier to spot errors or more important warnings in the verbose appveyor output.

See bitcoin#14086 (comment) plus discussion for context.

laanwj added a commit that referenced this pull request Sep 4, 2018

Merge #14094: refactoring: Remove unreferenced local variables
8ecaee1 Increase signal to noise in appveyor build output by reducing the MSVC warning count from 12 to 4 (12 is assuming the changes in #14086 are also implemented). (practicalswift)

Pull request description:

  Remove unreferenced local variables:

  Increase signal to noise in appveyor build output by reducing the MSVC warning count from 12 to 4. 12 is the number of MSVC warnings under our current appveyor setup assuming the changes in #14086 are also implemented.

  This makes it easier to spot errors or more important warnings in the verbose appveyor output. MSVC warnings are good, so having access to them in a noise free way (read: without trivial warnings) via appveyor without having to use Windows is really valuable.

  See #14086 (comment) plus discussion for context.

  Before:

  ```
  c:\projects\bitcoin\src\script\script.cpp(272): warning C4018: '>': signed/unsigned mismatch [C:\projects\bitcoin\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj]
  c:\projects\bitcoin\src\rest.cpp(467): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]
  c:\projects\bitcoin\src\test\allocator_tests.cpp(147): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\coins_tests.cpp(511): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\coins_tests.cpp(524): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\coins_tests.cpp(722): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\coins_tests.cpp(783): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\crypto_tests.cpp(535): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\dbwrapper_tests.cpp(265): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\net_tests.cpp(118): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\net_tests.cpp(151): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\scheduler_tests.cpp(57): warning C4305: 'argument': truncation from 'int' to 'bool' [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  ```

  After:

  ```
  c:\projects\bitcoin\src\script\script.cpp(272): warning C4018: '>': signed/unsigned mismatch [C:\projects\bitcoin\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj]
  c:\projects\bitcoin\src\test\allocator_tests.cpp(147): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\crypto_tests.cpp(535): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  c:\projects\bitcoin\src\test\scheduler_tests.cpp(57): warning C4305: 'argument': truncation from 'int' to 'bool' [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
  ```

Tree-SHA512: 5051134126c570b8421d57c710f1f1b977600398d2b5e69f8a8bd766b3696f992bf4e3459643b99a6b7e08dee1adc92985ee4d0d52b20755954415cb6f23f2fb

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Sep 4, 2018

Merge bitcoin#14086: appveyor: Use clcache to speed up build
e4a79b4 appveyor: Use clcache to speed up build (Chun Kuan Lee)

Pull request description:

  https://ci.appveyor.com/project/ken2812221/bitcoin/build/patch-4.407

  The build time reduced from 18 mins to 7 mins.

  - clcache is a third-party software, act much like ccache. (Compile-time cache)
  - `*.iobj` and `*.ipdb` is a MSVC built-in cache. (Link-time cache)

Tree-SHA512: b2f61730e23b85f36022f9088370dd50e0413b0dbb14e73e4e349165e3b4622508328d3e457b7f416fb2c42325c863243aeb92c7edf3af41482d8f8c9e239045

@MarcoFalke MarcoFalke merged commit e4a79b4 into bitcoin:master Sep 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ken2812221 ken2812221 deleted the ken2812221:patch-4 branch Sep 4, 2018

HashUnlimited added a commit to ToDoThings/bitcoin that referenced this pull request Sep 10, 2018

Increase signal to noise in appveyor build output by reducing the MSV…
…C warning count from 12 to 4 (12 is assuming the changes in bitcoin#14086 are also implemented).

This makes it easier to spot errors or more important warnings in the verbose appveyor output.

See bitcoin#14086 (comment) plus discussion for context.

Bushstar added a commit to Bushstar/Feathercoin that referenced this pull request Oct 9, 2018

Increase signal to noise in appveyor build output by reducing the MSV…
…C warning count from 12 to 4 (12 is assuming the changes in #14086 are also implemented).

This makes it easier to spot errors or more important warnings in the verbose appveyor output.

See bitcoin/bitcoin#14086 (comment) plus discussion for context.

jfhk added a commit to jfhk/bitcoin that referenced this pull request Nov 14, 2018

Increase signal to noise in appveyor build output by reducing the MSV…
…C warning count from 12 to 4 (12 is assuming the changes in bitcoin#14086 are also implemented).

This makes it easier to spot errors or more important warnings in the verbose appveyor output.

See bitcoin#14086 (comment) plus discussion for context.

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 26, 2018

Increase signal to noise in appveyor build output by reducing the MSV…
…C warning count from 12 to 4 (12 is assuming the changes in bitcoin#14086 are also implemented).

This makes it easier to spot errors or more important warnings in the verbose appveyor output.

See bitcoin#14086 (comment) plus discussion for context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.