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 integration #89

Merged
merged 1 commit into from
Aug 19, 2017
Merged

AppVeyor integration #89

merged 1 commit into from
Aug 19, 2017

Conversation

dvolgyes
Copy link
Contributor

AppVeyor is a continuous integration platform, similar to Travis CI but it runs on Windows platforms.
This pull request adds configuration file to STIR.
The workflow for the integration:

  • opening appveyor account (just log in with github)
  • add project from github and grant permissions
  • merge the pull request
  • check the AppVeyor results

At this moment the windows tests seem to be outdated compared to the linux tests.
So theoretically the scripts are called, but this part should be refined later.
However, the compilation part should work and it should detect build errors.

Current compilers: Visual Studio 2013 and 2015 with 32bit and 64bit support, but this list could be extended with a lot of other compilers (VS 2017, gcc, clang, etc.)
For the complete feature set, see the AppVeyor documentation:
https://www.appveyor.com/docs/

One strange feature of the AppVeyor builds: the version number should be set in the config file, at this moment I set 3.1, so the builds will be 3.1.1, 3.1.2 and so on.

@KrisThielemans
Copy link
Collaborator

great!

why are we not running ctest? If that doesn't work, it can be done via

msbuild RUN_TESTS.vcxproj

Not sure if you need the version number really in there. See https://github.com/UCL/PETPVC/blob/master/appveyor.yml. @bathomas, can you advise?

@dvolgyes
Copy link
Contributor Author

Build number: you are right, probably we do not need it.
(Longer answer: because it is my first AppVeyor adventure, and the documentation I read did not give a definite answer, so I just modified a template which had major and minor versions too.)

Test: good point, I try to fix the test. (ctest: I am not quite sure about the invocation of ctest in windows, so I don't promise anything.)

@dvolgyes
Copy link
Contributor Author

By the way, if we change the README.txt to README.md, then it will be processed as a markdown file, and we could include links and badges, e.g. to the CI/code coverage sites. What do you think?

@KrisThielemans
Copy link
Collaborator

KrisThielemans commented Aug 17, 2017 via email

@dvolgyes
Copy link
Contributor Author

Let's try it. AppVeyor is slow (around 30 mins, but in a few hours we will see both msbuild and ctest results.) (Travis will obviously pass, there is no change there.)
ctest/msbuild: I did not use any of them, so i just started with a random order.

By the way, in a separate pull request, we could try integrate Coveralls.io, which tracks code coverage (which source lines are tested by the different tests). It is callable from Travis, and it generates code coverage reports, like this:
https://coveralls.io/github/dvolgyes/perceptionmd?branch=master
(I only used it for python so far, but there is C++ support with gcov, so it should work.)

@dvolgyes
Copy link
Contributor Author

It took a while, but I think the current version should work nicely.
To my surprise, it turned out that CMake generates multi-target projects for MS VS, and single-target (Makefile-like) projects for unices. This wasn't an issue for the standard compilation but became problematic for the ctest execution. See the CMakeList.txt update, and the StackOverflow link there.

So if this https://ci.appveyor.com/project/dvolgyes/stir/history
shows successful build in the morning, then I consider the pull request complete.
(Badge: AppVeyor has a badge section in its menu, the link from there should be copied into the README.md but this can only be done by the project owner, because it contains a unique id.)

@dvolgyes
Copy link
Contributor Author

Apparently, Travis cannot keep the pace, so I think all but the last build should be stopped. (Otherwise it will test unnecessary commits for one more day.)

@dvolgyes
Copy link
Contributor Author

Somehow the OSX builds on Travis are failing, but it should not.
There was a maintenance yesterday, and I am pretty sure this is the reason.
(I did not change any related file, it must not affect Travis. The only part changed is on Travis' side.)
We will see in a few hours.

@KrisThielemans
Copy link
Collaborator

All looks good, except the changes to CMakeLists.txt. I've added comments there. Do you really need some of those changes for the Appveyor build? I don't need it on Windows for sure... If we don't need them, I'd rather have them in a separate PR and revert CMakeLists.txt here.

@KrisThielemans
Copy link
Collaborator

Hi Daivid, let's not muddle the waters with more travis changes. Let me know when this is ready.

@bathomas can you have a quick look at the Appveyor.yml?

@bathomas
Copy link
Collaborator

Looks good to me! Only one thing you might want to consider is whether you want to build every commit. Maybe only tags or perhaps master only?

@KrisThielemans
Copy link
Collaborator

travis is now failing with ROOT on MacOS:

-- CERN ROOT not found. If you do have it, set the missing variables (missing:  CERN_ROOT_LIBRARIES CERN_ROOT_INCLUDE_DIRS) (found version "030100")

I suggest we revert .travis.yml and open a different PR for this

@KrisThielemans
Copy link
Collaborator

@bathomas, we currently run travis at every commit as well. not sure how to do this otherwise without taking up a lot of my time :-;

@dvolgyes
Copy link
Contributor Author

dvolgyes commented Aug 18, 2017

AppVeyor:
I had quite bad luck in the last 24 hours.
First of all, I spent around half a day to find solution for the ctest/cmake error.
One of the tests failed until I came up with the cmakelist patch, and I was quite certain in
the solution.
Now, the issue disappeared and it works nicely without any modification.

Travis:
The ROOT package has been updated, and the latest ROOT 6.10 cannot be compiled without proper C++11 support. (And probably there are some other minor changes.) For the sake of the test, I fixed ROOT5 for the osx build.

Maybe we should define somewhere what is the recommended version of BOOST, ROOT, CMake, gcc, clang, etc.

Anyway: if we revert travis, it will fail for macos. Actually, if you re-run any previous successful build, it will fail because of the ROOT update in homebrew.
My latest commit installs ROOT5, but the test still fails (with root 5.34), I have no idea why.
We can split this into two PR.

@dvolgyes
Copy link
Contributor Author

About the per-commit build: I would support the current setup. Yes, in some cases, like the last PR, it might need around a dozen build. But most of the time it catches the bugs early, and it is easier to fix immediately. (And if there is no bug, then it will be just tested one. I had a lot of builds because the PR was imperfect.) There are several months, sometimes even years between full releases. Tags could be produced, release candidates, etc., but I don't really see the point. Travis/AppVeyor are free, and if time is a constraint, then any build can be stopped, so the queue could be sped up. Long story short: I support the per-commit builds.

@dvolgyes
Copy link
Contributor Author

I removed the Travis changes. Due to the OSX package changes, this will cause some Travis builds to fail. Ignore it, please, and let's fix it in another PR.

@KrisThielemans
Copy link
Collaborator

Thanks David!
let me know what I need to do for AppVeyor. It's probably not as simple as adding
https://ci.appveyor.com/api/projects/status/github/UCL/STIR to the README.md...

@dvolgyes
Copy link
Contributor Author

Hi,
It is almost that simple.

  • go to AppVeyor
  • login with github (no other registration needed)
  • add new project -> select UCL/STIR repository
  • it will ask for permissions, grant them
  • go to the repository settings
  • 3rd from the bottom in the menu: Badges
  • copy the Markdown line to the README.md
  • commit README.md, push it
  • check the results in an hour

Badges:

  • There are two examples: current build and master branch.
    Honestly, I do not see any difference. However, you can easily modify the master branch to any other branch, if you wish.
    E.g. some projects have three badges:
  • current master branch (development branch)
  • current stable branch
  • old stable branch

So if there is a bugfix in the master which is backported, then the stable branches will change, and AppVeyor will test them. (The same is true for Travis too, but at this moment this isn't really an issue.)

Settings in general:
Pull requests: they will be automatically tested, and config is stored in the appveyor.yml.
Be careful, sometimes the same things could be changed in the GUI and in the appveyor file too.
(I think the file has higher priority, actually.) Most settings are trivial and the default is fine, but you can read them, if there is a feature you would like to play with. (E.g. successful builds could generate binary blobs for downloads, etc.)

I think we can close this PR when you commit the README.md update. (You are the only one who can actually access the badge, so it must be you.) In another PR, we can try to fix OSX builds with ROOT which is broken at this moment.

@KrisThielemans
Copy link
Collaborator

Thanks David. As I had merged the PR already, I've commited the change to README.md directly on master. All done now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants