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

Initial Appveyor CI integration #2083

Merged
merged 2 commits into from
Dec 8, 2017

Conversation

SergioRAgostinho
Copy link
Member

It's far from mimicking what we have on Travis but it's always an additional check.

Closes #2078

@SergioRAgostinho SergioRAgostinho added this to the pcl-1.9.0 milestone Nov 14, 2017
@SergioRAgostinho
Copy link
Member Author

SergioRAgostinho commented Nov 14, 2017

@VictorLamoine why is this triggering a hook to your Appveyor account? 😅

Edit:

  • Answering my own question, you were the first trying the integration and the repo got associated to your project's id (in Appveyor).
  • In order to do this properly there should be an Appveyor user to represent the organization, created with these instruction https://www.appveyor.com/docs/team-setup/ . Ping @jspricke @rbrusu

README.md Outdated
@@ -14,6 +14,7 @@ Continuous integration
[license]: https://github.com/PointCloudLibrary/pcl/blob/master/LICENSE.txt

[![Build Status](https://travis-ci.org/PointCloudLibrary/pcl.svg?branch=master)](https://travis-ci.org/PointCloudLibrary/pcl)
[![Build Status](https://ci.appveyor.com/api/projects/status/PointCloudLibrary/pcl/branch/master?svg=true)](https://ci.appveyor.com/project/PointCloudLibrary/pcl)
Copy link
Member Author

Choose a reason for hiding this comment

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

This will need to be updated later

@UnaNancyOwen
Copy link
Member

Is it possible to test with multiple versions of Visual Studio?
I will always release All-in-one Installer the latest version and the one previous version for Visual Studio.
I think that it is preferable to be tested with Visual Studio 2017 and Visual Studio 2015.

image:
- Visual Studio 2015
- Visual Studio 2017

(BTW: Currently, PCL can not be built with Visual Studio 2017 (15.3-15.4), because Visual C++ 2017 compiler have bug. It will be fixed in the next version (15.5). Please read this bug report.)

@SergioRAgostinho
Copy link
Member Author

Is it possible to test with multiple versions of Visual Studio?

No doubt. This should give you an idea of all you can do https://www.appveyor.com/docs/appveyor-yml/

.appveyor.yml Outdated
- c:\tools\vcpkg\installed\

install:
- vcpkg install boost eigen3 flann qhull
Copy link
Member

Choose a reason for hiding this comment

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

If vcpkg update is available, I think that it able to be add vtk to installation list.
vtk port stabilized with recent commits.

Copy link
Member Author

Choose a reason for hiding this comment

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

The biggest struggle here is to manage to build everything under 1 hour. You might be able to do it, but we might not be able to perform the build job under that time.

Copy link
Member

Choose a reason for hiding this comment

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

Some dependent libraries of VTK port are optional now.
For example, Qt5 has become an option. It was took a lot of time to build.
Now, It is not necessary, because VTK port build without Qt5 by default.
Probably, It has cut down a lot of time.

@SergioRAgostinho
Copy link
Member Author

Ok, right now if I add vtk as a dependency it fails because the link to libpng appears to be broken or down. I ran vcpkg update before so I would say for now we can't really include it. More info: https://ci.appveyor.com/project/SergioRAgostinho/pcl/build/1.0.23/job/j4ug5u3ldc5yx0o8#L383

@UnaNancyOwen
Copy link
Member

Sorry, AppVeyor seems that vcpkg update can't be used.

vcpkg update
Using local portfile versions. To update the local portfiles, use `git pull`.
No packages need updating.

@taketwo
Copy link
Member

taketwo commented Nov 15, 2017

I believe you need to add something like this before update:

- cd c:\Tools\vcpkg
- git pull
- .\bootstrap-vcpkg.bat

@SergioRAgostinho
Copy link
Member Author

Btw, we now have two webhooks into Appveyor, one from Jochen and one from Victor. We should clean this in the end.

.appveyor.yml Outdated
@@ -17,7 +14,6 @@ build:
parallel: true

build_script:
- cd c:\project\pcl
Copy link
Member

Choose a reason for hiding this comment

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

BTW this should be c:\projects\pcl (plural)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. It never got there though 😢

@SergioRAgostinho
Copy link
Member Author

I'm not gonna be doing anything more for the time being. The 2017 image is failing as expected.

@UnaNancyOwen
Copy link
Member

UnaNancyOwen commented Nov 16, 2017

It seems that QHull installation step has failed on Visual Studio 2017 image. (In Visual Studio 2017 image, It should fail in PCL building step.)
It has been successful in the latest vcpkg (when execute git pull and .\bootstrap-vcpkg.bat) in history 1.0.4. (The cause of failure in history 1.0.4 is a path mistake. It is "c:\projects\pcl" not "c:\project\pcl".)
I think it is necessary step.

install:
+  - cd c:\tools\vcpkg
+  - git pull
+  - .\bootstrap-vcpkg.bat
   - vcpkg install boost eigen3 flann qhull
build_script:
+  - cd c:\projects\pcl
   - MKDIR build
   - CD build
   - cmake   -DCMAKE_TOOLCHAIN_FILE=c:/tools/vcpkg/scripts/buildsystems/vcpkg.cmake
                   -DPCL_SHARED_LIBS=ON
                   -DPCL_BUILD_WITH_BOOST_DYNAMIC_LINKING_WIN32=ON
                   -DPCL_BUILD_WITH_FLANN_DYNAMIC_LINKING_WIN32=ON
                   -DPCL_NO_PRECOMPILE=ON
                   -DBUILD_simulation=ON
                   -DBUILD_global_tests=OFF
                   -DBUILD_examples=OFF
                   -DBUILD_tools=OFF
                   -DBUILD_apps=OFF
                   ..
   - cmake --build . --config Release

@SergioRAgostinho
Copy link
Member Author

We know that 2017 is gonna fail anyway, so 2015 is the only usable one for the time being. If I update vcpkg I'm not able to do anything with 2015 anymore. That's why I'm not updating it.

@UnaNancyOwen
Copy link
Member

2015: Fails to build QHull after updating vcpkg https://ci.appveyor.com/project/JochenSprickerhof/pcl/build/1.0.4/job/grcx2i3b8f0k3veb

Hmm, I tried the same version on local environment, but this problem could not be reproduced.
I will try it in AppVeyor environment at a later date.

@UnaNancyOwen
Copy link
Member

@SergioRAgostinho I don't know why failed vcpkg install qhull in history 1.0.4, but it seems that it will succeed now. https://ci.appveyor.com/project/UnaNancyOwen/appveyor/build/1.0.16

.appveyor.yml Outdated
@@ -14,8 +18,9 @@ build:
parallel: true

build_script:
- MKDIR build
- CD build
- cd c:\projects\appveyor
Copy link
Member

Choose a reason for hiding this comment

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

-  - cd c:\projects\appveyor
+  - cd c:\projects\pcl

@UnaNancyOwen
Copy link
Member

UnaNancyOwen commented Nov 17, 2017

This CI seems to works properly. It is normal that build failed on Visual Studio 2017 image.

@UnaNancyOwen
Copy link
Member

UnaNancyOwen commented Nov 17, 2017

(my perception may be wrong because I'm not familiar with linux...)
I think that PCL build and test with x64 on Travis CI.
If so, I think that PCL should build and test with x64 also on Windows.

vcpkg can specify x64 triplet (vcpkg install boost:x64-windows).

 install:
   - cd c:\tools\vcpkg
   - git pull
   - .\bootstrap-vcpkg.bat
   - vcpkg version
-  - vcpkg install boost eigen3 flann qhull
+  - vcpkg install boost:x64-windows eigen3:x64-windows flann:x64-windows qhull:x64-windows

In addition, It is necessary to specify x64 generator (-G"Visual Studio 14 2015 Win64", -G"Visual Studio 15 2017 Win64").

- image:
-   - Visual Studio 2015
-   - Visual Studio 2017
+ environment:
+    matrix:
+      - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
+        PLATFORM: x64
+        GENERATOR: "Visual Studio 14 2015 Win64"
+      - APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017
+        PLATFORM: x64
+        GENERATOR: "Visual Studio 15 2017 Win64"
-   - cmake   -DCMAKE_TOOLCHAIN_FILE=c:/tools/vcpkg/scripts/buildsystems/vcpkg.cmake
+   - cmake   -G"%GENERATOR%"
+             -DCMAKE_TOOLCHAIN_FILE=c:/tools/vcpkg/scripts/buildsystems/vcpkg.cmake

(If adopt this proposal, I think that it is good to clear the cache once. and rebuild cache.)

@SergioRAgostinho
Copy link
Member Author

(If adopt this proposal, I think that it is good to clear the cache once. and rebuild cache.)

No worries, appveyor doesn't preserve cache between different environmental values.

@jspricke
Copy link
Member

I had a second look into this and added a proper team account (at least I hope so). Please ping me, if I need to change anything.

@SergioRAgostinho
Copy link
Member Author

@jspricke Can you add me to the team account? My username in appveyor is also SergioRAgostinho.

@jspricke
Copy link
Member

I updated the rights for all PCL Admins, ping me if you need more.

@SergioRAgostinho
Copy link
Member Author

I can't seem to restart/cancel jobs if needed and I know I can do that in my account for my personal fork. :/

@SergioRAgostinho
Copy link
Member Author

FYI guys, you'll need to logout and login from appveyor to be able to admin the account.

@jasjuang
Copy link
Contributor

This is great!

@SergioRAgostinho
Copy link
Member Author

I think now it's a matter of finding a random commit in vcpkg's history in which all dependencies work properly :D

@UnaNancyOwen
Copy link
Member

I think so too.

@SergioRAgostinho
Copy link
Member Author

Can you help with me that Tsukasa?

@SergioRAgostinho
Copy link
Member Author

👍 The last patch sounds good to me. Do you have any idea why are we forcing the static linking of QHull on windows, in the first place?

@UnaNancyOwen
Copy link
Member

I don't know why PCL (currently version) always find static link library (qhullstatic.lib) of QHull on Windows.
But, I confirmed that PCL port can build with dynamic link library (qhull_p.lib) of QHull. Therefor, I think that is not a necessary condition on Windows.

.appveyor.yml Outdated
@@ -71,7 +70,7 @@ build_script:
-DBUILD_simulation=ON
-DBUILD_global_tests=OFF
-DBUILD_examples=OFF
-DBUILD_tools=OFF
-DBUILD_tools=ON
Copy link
Member

Choose a reason for hiding this comment

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

BUILD_tools will take a long time to build.

octree.begs [offset + i] = cell_begs[i];
octree.ends [offset + i] = cell_begs[i + 1];
octree.codes[offset + i] = parent_code_shifted + cell_code[i];
// octree.begs [offset + i] = cell_begs[i];
Copy link
Member

Choose a reason for hiding this comment

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

Multitasking? 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️

.appveyor.yml Outdated
@@ -43,7 +43,9 @@ install:
- set
- cd c:\tools\vcpkg
- git pull
- git checkout 053f3ee32f309208ac3af9c71b95253d4a144436
# - git checkout 053f3ee32f309208ac3af9c71b95253d4a144436
- echo.set(VCPKG_BUILD_TYPE release)>> C:\Tools\vcpkg\triplets\%PLATFORM%-windows.cmake
Copy link
Member

Choose a reason for hiding this comment

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

Can you point at the source of this hint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tip given here microsoft/vcpkg#143 (comment) by the maintainer (which was kind enough to commit this microsoft/vcpkg@5335d17 right before ^^)

Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, cool!

@UnaNancyOwen
Copy link
Member

UnaNancyOwen commented Nov 26, 2017

This pull request microsoft/vcpkg#2258 has been merged.
If you want to use new commit of QHull, Please apply this fix (cmakelists.patch) to upstream.

@SergioRAgostinho
Copy link
Member Author

SergioRAgostinho commented Nov 28, 2017

@UnaNancyOwen is zlib working fine in your dev environment? I mean, can you download it and install it from vcpkg?

@UnaNancyOwen
Copy link
Member

UnaNancyOwen commented Nov 28, 2017

@SergioRAgostinho I confirmed that zlib will failed install with v140 toolset (Visual Studio 2015).
I don't know why failed last time, but It succeeded when try again. Please try again with master/HEAD.

@UnaNancyOwen
Copy link
Member

UnaNancyOwen commented Dec 5, 2017

Visual Studio 2017 15.5 has been released.
And, I confirmed that PCL can be built successfully on this version.
We will be able to enable testing on Visual Studio 2017 image when environment of AppVeyor is updated.
appveyor/ci#1963
https://www.appveyor.com/updates/

@SergioRAgostinho
Copy link
Member Author

This is good for merge now. Once the new visual studio is released to AppVeyor, uncomment the corresponding lines.

@UnaNancyOwen
Copy link
Member

UnaNancyOwen commented Dec 6, 2017

LGTM 👍
I think this pull request is ready to merge.
In the future, We can challenge to running tests on Windows.
microsoft/vcpkg#2261

@taketwo What do you think?

.appveyor.yml Outdated
- mkdir build
- cd build
- cmake -G"%GENERATOR%"
-DCMAKE_TOOLCHAIN_FILE=c:/tools/vcpkg/scripts/buildsystems/vcpkg.cmake
Copy link
Member

Choose a reason for hiding this comment

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

Use %VCPKG_DIR% here?

.appveyor.yml Outdated
init:
- cd %VCPKG_DIR%
- git pull
# - git checkout 94bd9dd66e9db88f965c8b270ea58f927685a317
Copy link
Member

Choose a reason for hiding this comment

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

Is the intention to uncomment it later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not this time. Just a left over.

@SergioRAgostinho
Copy link
Member Author

Somewhere down the road we need to stop using this git pull/bootstrap strategy and just stick with the version provided by default in appveyor. The current strategy doesn't allow us to make use of the caching mechanisms implemented for vcpkg in appveyor.

@taketwo taketwo removed the needs: more work Specify why not closed/merged yet label Dec 8, 2017
@taketwo taketwo merged commit 9517e07 into PointCloudLibrary:master Dec 8, 2017
@SergioRAgostinho SergioRAgostinho deleted the appveyor branch December 8, 2017 14:38
@SergioRAgostinho
Copy link
Member Author

Cache is actually working properly 🎉

This screen is very satisfying :)

screenshot from 2017-12-08 21-46-25

@UnaNancyOwen
Copy link
Member

I shared this news to vcpkg team!
They are really happy this news. 😄

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

Successfully merging this pull request may close these issues.

5 participants