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

ci: Add Appveyor CI #13964

Merged
merged 3 commits into from Aug 15, 2018

Conversation

Projects
None yet
10 participants
@ken2812221
Copy link
Member

commented Aug 14, 2018

Introduce Appveyor CI for MSVC. This would require the owner adding appveyor to this repo. Also fix some MSVC incompatible code.

This appveyor.yml file is modified from @sipsorcery and @NicolasDorier 's code in #12613.

Appveyor CI result: https://ci.appveyor.com/project/ken2812221/bitcoin/build/1.0.151

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

Concept ACK

More diversity in testing is better.

FWIW, it seems like a few tests are missing from test_bitcoin.vcxproj – is that intentional?

$ for FILE in $(git ls-files -- "*_tests.cpp"); do FILE_BASENAME=$(basename ${FILE}); grep -q ${FILE_BASENAME} build_msvc/test_bitcoin/test_bitcoin.vcxproj || echo "* ${FILE_BASENAME} missing in test_bitcoin.vcxproj"; done
* descriptor_tests.cpp missing in test_bitcoin.vcxproj
* key_io_tests.cpp missing in test_bitcoin.vcxproj
* merkleblock_tests.cpp missing in test_bitcoin.vcxproj
* txvalidation_tests.cpp missing in test_bitcoin.vcxproj
@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

@practicalswift I'm not sure if it is intentional. Maybe @sipsorcery forgot to add them. I'll add those files after the test pass.

@ken2812221 ken2812221 force-pushed the ken2812221:fix-msvc branch Aug 14, 2018

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

btw, msbuild support wildcard replacing all ..\..\src\test\xxxxx_tests.cpp by one ..\..\src\test\*.cpp might work.

EDIT: Or maybe ..\..\src\test\*_tests.cpp

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

utACK a87072f4b9ce2e4020dd2a1ca4ed7cb227757090

Could you please post instructions for maintainers how to enable this appveyor thing?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

I guess https://ci.appveyor.com/projects/new on "Public GitHub repositories"?

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

@MarcoFalke Yes, note that appveyor does not connect with github team. So the owner need to change his account name on appveyor to "bitcoin" to get "bitcoin" project prefix(https://ci.appveyor.com/account), but that is not necessary.

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

Tested ACK a87072f

@NicolasDorier's suggestion is a good one and would help avoid files missing in the future (it was human error on my behalf that caused them to be missing in the first place).

<ItemGroup>
  <ClCompile Include="..\..\src\test\*.cpp" Exclude="..\..\src\test\test_bitcoin_fuzzy.cpp" />
  <ClCompile Include="..\..\src\wallet\test\*.cpp" />
  </ItemGroup>

@MarcoFalke if there is a webhook on the bitcoin/bitcoin project under Settings->WebHooks and we can get it that would allow us to initiate the AppVeyor builds automatically whenever there is a commit. At the moment I kick off a build whenever I see a Twitter commit message.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

@sipsorcery Would it be possible to use wildcards in the other project files to make the maintenance of these scripts easier? :-)

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

It must run on pull requests. Otherwise we can't enable it on the master branch without having to fix each failure post merge.

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

@practicalswift That might not happen on other project files because that would cause link error.

@MarcoFalke Yes, it can run on pull request https://ci.appveyor.com/project/ken2812221/bitcoin/build/1.0.151

@ken2812221 ken2812221 force-pushed the ken2812221:fix-msvc branch to 1f6ff04 Aug 14, 2018

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

@practicalswift For the other projects it's not really possible to use wildcards if we also want to maintain the same build components as the Makefile. In particular libbitcoin_common, libbitcoin_util, libbitcoin_consensus and libbitcoin_server all include *.cpp files from /src.

One alternative solution would be to write a script that parses the Makefile to automatically build the Visual Studio project files. I'll put it on the list :-).

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

Tested ACK 1f6ff04

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Aug 15, 2018

Merge bitcoin#13964: ci: Add Appveyor CI
1f6ff04 Use wildcard path in test_bitcoin.vcxproj (Chun Kuan Lee)
90cc69c ci: Add appveyor.yml to build on MSVC (Chun Kuan Lee)
4d0c792 Make macro compatible with MSVC (Chun Kuan Lee)

Pull request description:

  Introduce Appveyor CI for MSVC. This would require the owner adding appveyor to this repo. Also fix some MSVC incompatible code.

  This `appveyor.yml` file is modified from @sipsorcery and @NicolasDorier 's code in bitcoin#12613.

  Appveyor CI result: https://ci.appveyor.com/project/ken2812221/bitcoin/build/1.0.151

Tree-SHA512: b5b0f1686a33e54325ea6de81606806a7d9a0f8d4acbb97c9ce598386e8fcb2220def264777609ed2b850ac8c490fd181303ea522c5a70487272d46995f4c52d

@MarcoFalke MarcoFalke merged commit 1f6ff04 into bitcoin:master Aug 15, 2018

1 check passed

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

@ken2812221 ken2812221 deleted the ken2812221:fix-msvc branch Aug 15, 2018

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

A small note on the AppVeyor build in this PR.

The URL to install the bzip dependency in the AppVeyor version of vcpkg (the Microsoft C++ dependency that makes building bitcoin-core straight forward) is currently broken as per microsoft/vcpkg#4046.

The consequence is that anyone that forks bitcoin-core and creates their own AppVeyor job will end up with a failing job. At some point AppVeyor will update their vcpkg version and this will be fixed. In the meantime it's possible to workaround the problem by manually updating the vcpkg files using:

cd c:\tools\vcpkg
git pull

I don't think it's worth adding these commands to the appveyor.yml as it would potentially result in dependency updates that a user may not be expecting.

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

@sipsorcery maybe pulling and just ckecking out specifc commit so that updates on appveryor does not have impact.

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

Before doing a PR I'll try and ascertain how/when AppVeyor update vcpkg https://help.appveyor.com/discussions/problems/15817-vcpkg-updates.

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

@MarcoFalke Did you have any trouble while setting up Appveyor CI?

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

Cool they replied on https://help.appveyor.com/discussions/problems/15817-vcpkg-updates

install:
- cmd: |
    cd "C:\Tools\vcpkg"
    git pull
    .\bootstrap-vcpkg.bat
    cd %appveyor_build_folder%

@MarcoFalke , you will need that for now if you want to include to github. That said they will fix in few weeks.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

@laanwj @ken2812221 I have set this up through DrahtBot. Though, it wouldn't run on pull requests, it seems.

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

@MarcoFalke Can you take a look at https://github.com/bitcoin/bitcoin/settings/hooks
It should enable pull request
image

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

Merge #13997: appveyor: fetch the latest port data
ea16c2d appveyor: fetch the latest port data (Chun Kuan Lee)

Pull request description:

  Discussion in #13964 (comment) , fetch the latest port data before installing it.

Tree-SHA512: a3b95aeb3b298130ff0617a8dec5c97c0882cf7a3b72ce792e63d8f2c2ac4a297dfa0d3357878c2198a9fea62d0f24df56598293dde88963dd043e121be4dc3a
@ras0219-msft

This comment has been minimized.

Copy link

commented Aug 17, 2018

@sipsorcery We would definitely recommend what @NicolasDorier suggests -- the best fix here is to checkout a specific vcpkg git commit in your appveyor.yml:

cd C:\Tools\vcpkg
git checkout <some sha, perhaps today's master>
.\bootstrap-vcpkg.bat
cd %appveyor_build_folder%

Whenever you want to update your dependencies (say, to fix issues like this), you just need to change the commit id.

@donald1stan

This comment was marked as spam.

Copy link

commented on 1f6ff04 Aug 22, 2018

i would like to introduce Bitcoin Mining to you, this is a platform where you get to invest and earn from your investment. Using Bitcoin you can make up to $10,000 weekly and more than $100,000 in a month, by investing:

$500 you get $1700 weekly
$1000 you get $3200 weekly
$2000 you get $6200 weekly
$5000 you get $15600 weekly so on, it depends on how much you are willing to invest. Also if you have issues accessing your wallet,

Please note that you don't require any special skills and it's doesn't require much of your time as you can do this anywhere and anytime. For further details contact donald1stan@gmail.com

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Hmm, that would make it impossible to run multiple pull requests?

@ken2812221

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

@MarcoFalke If you push to the same PR, it would cancel the previous one and does not affect other PRs.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Post merge: I'm unsure about the usefulness of this. This requires documentation. I have no clue how to make #14032 pass AppVeyor...
Somehow I have the feeling that this sets an unexpected heavy burden on those not primarily interested in MSVC support.

IMO MSVC compatibility should be maintained by those interested in it.

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

The project files for the msvc build are currently not automatically synchronised with the Makefiles. Whenever a new class file is added or removed the msvc build will break. Until we write a script to keep them in sync I agree with @jonasschnelli in that blocking a PR based on the msvc build is not ideal.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

@sipsorcery Agreed!

Could a way to solve this be to auto-generate the project files (or at least the parts referencing files) to make sure they’re automatically changed when new files are added/removed?

The only input needed for such auto-generation would be find src/ -type f? :-)

See previous discussion in #11526 (comment) and #11526 (comment) about auto-generation.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

blocking a PR based on the msvc build is not ideal.

From irc yesterday (https://botbot.me/freenode/bitcoin-core-dev/msg/103712220/), it sounds like appveyor failures will not actually block merges, so they might not be a real problem (though annoying to look at).

Could a way to solve this be to auto-generate the project files

An easier way to alleviate this might just be to use wildcards more places in the project files. Autogeneration could help or hurt, depending how it is implemented.

@sipsorcery

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

The options I can think of:

  1. Leave the Visual Studio project files as is and manually update the project files as class files are added and removed. Over the last 12 months I’d estimate I’ve updated the project files about a dozen times,
  2. Investigate switching to a wildcard project file,
  3. Switch from Makefile to CMake to allow a wider variety of platform builds from a single configuration.

For my own purposes option 1 is by far the most useful. Having a Visual Studio project configuration that directly reflects the current bitcoin core libraries makes building and debugging straight forward and is the by far the biggest benefit for me. It’s also not a great burden to update the project files when the build breaks but that’s based on only building the master branch.

For options 2 and 3 it would make building on Windows easier but be worse for debugging and comprehending the code.

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Oct 24, 2018

Merge bitcoin#14559: appveyor: Enable multiwallet tests
4dca7d0 appveyor: Enable multiwallet test (Chun Kuan Lee)

Pull request description:

  Based on bitcoin#14320

  This PR enable multiwallet test on appveyor. Also re-enable symlink tests on Windows which is available after Windows Vista.

  I disable these tests in bitcoin#13964 because I suppose that Windows does not support symlink, but I was wrong.

Tree-SHA512: 852cd4dedf36ec9c34aff8926cb34e6a560aea0bb9170c7a2264fc292dbb605622d561568d8df39aeb90d3d2bb700901d218ea7e7c5e21d84827c40d6370b369
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.