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

Cross-platform VTK 7 build #1

Merged
merged 18 commits into from Jun 30, 2016
Merged

Cross-platform VTK 7 build #1

merged 18 commits into from Jun 30, 2016

Conversation

Korijn
Copy link
Contributor

@Korijn Korijn commented Jun 24, 2016

  • Increment build number to 1
  • Build for all platforms
  • Skip 32-bit builds to reduce build time
  • Removed unused INSTALL_PKGCONFIG_DIR option from build.sh

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@Korijn
Copy link
Contributor Author

Korijn commented Jun 24, 2016

Looks like OS X is now building as well. Build timeouts will still be an issue though, so I tried to pre-emptively skip less popular configurations.

@ccordoba12
Copy link
Contributor

Why AppVeyor is not getting triggered?

@ivoflipse
Copy link
Contributor

@ccordoba12 because it was taking too long, running into the limits of AppVeyor

@jakirkham
Copy link
Member

So, @Korijn, when one changes the skip setting, one needs to re-render with conda-smithy. Please install conda-smithy in the root environment from conda-forge and run conda smithy rerender in the top level directory of this repo. Commit and push all changes. That should enable AppVeyor support.

@grlee77
Copy link
Member

grlee77 commented Jun 24, 2016

looks like Travis timed out at 80% on the build. Maybe adding -j2 to the make line would run the build in parallel and allow it to complete. I don't know if it is guaranteed that we can use 2 cores on Travis though?

@grlee77
Copy link
Member

grlee77 commented Jun 24, 2016

To answer my own question above:
unfortunately, the specs for Os X builds on Travis only list 1 core:
https://docs.travis-ci.com/user/ci-environment/

@ccordoba12
Copy link
Contributor

@ccordoba12 because it was taking too long

Yes, but you skipped win32 so win64 should work. So you need to follow @jakirkham advice to update the skip rules :-)


@jakirkham, this is the perfect example to ask for more time to Travis and AppVeyor :-)

@Korijn
Copy link
Contributor Author

Korijn commented Jun 24, 2016

I was wondering about that @jakirkham! I will get to it later. It's getting late on this side of the globe. :) Thanks for your input! I'll try enabling multicore builds for linux and os x using $CPU_COUNT as well.

Also, what do you think about raising some funds to upgrade the CI packages? I think it shouldn't be too much of a stretch...

@jakirkham
Copy link
Member

jakirkham commented Jun 24, 2016

No worries, @Korijn. If I have time some day, I'll try to write some docs. 😄

Yeah, Travis CI says it has only 1 core. So, not sure what we can do about it.

Unfortunately, the number of cores is often incorrectly reported by CI and may be reporting the number of cores on a shared node. In short, I wouldn't recommend using $CPU_COUNT.

@jakirkham, this is the perfect example to ask for more time to Travis and AppVeyor :-)


Also, what do you think about raising some funds to upgrade the CI packages? I think it shouldn't be too much of a stretch...

I think this is a great idea, @Korijn @ccordoba12.

There have been people that have periodically mentioned interest in donating build time. This certainly is a possibility, but really hasn't gotten the attention it probably deserves. We would probably want to figure out some terms for acceptable use and code necessary to make this function.

There has also been some discussion about becoming a NumFOCUS sponsored group. It seems NumFOCUS would be interested in having us and I haven't heard any dissent on our side, but it hasn't been discussed in detail. This would also be another path for acquiring such funds. It also might lower the administrative overhead.

Writing letters are probably the easiest. If people want to start on a letter, please go for it. We did something similar with GitHub in the past. My suggestion is someone start a Google Doc and open editing to everyone. Then just drop this in a new issue on the web repo issue tracker. I can't promise that I will get to it myself any time soon. However, I'm more than happy to participate and guide the process along.

@Korijn
Copy link
Contributor Author

Korijn commented Jun 25, 2016

I rerendered for the new skip rules, and added what is supposed to configure cl on Windows to use as many cores as are available on AppVeyor (which is 2).

Some surprising output on the Windows build:

CMake Warning:
  Manually-specified variables were not used by the project:
    INSTALL_BIN_DIR
    INSTALL_LIB_DIR
    INSTALL_MAN_DIR

After reading a bit more, these variables are used when VTK also compiles and installs 3rd party, libs such as zlib. Currently the build is not configured to do so (I think), but I propose leaving them in anyway.

@Korijn
Copy link
Contributor Author

Korijn commented Jun 26, 2016

Everything timed out! :') Most reached into 90% though. Maybe there's some optional components we can skip...

@patricksnape
Copy link
Contributor

You can definitely build VTK on Appveyor but you need to ask for 1.5 hour build times.

@Korijn
Copy link
Contributor Author

Korijn commented Jun 26, 2016

  • AppVeyor reaches 84% for Python 2.7 and 57% for Python 3.5. Build time extension (to 1.5x) request is pending.
  • TravisCI reaches 87%.
  • CircleCI doesn't parallelize across a build matrix apparently. It completes the first build, then reaches 97% for the second, so this build needs a bit more than 1.5x the current build time to do all 3 configurations we have opted for here.

@patricksnape I emailed AppVeyor about it. Can we do the same thing for CircleCI and TravisCI?

@demarle
Copy link

demarle commented Jun 27, 2016

Thanks for working on this! If build times are still a limiting factor, is it possible to switch to ninja? (cmake -G Ninja, then ninja instead of make or nmake). In VTK/PV developer land we've all pretty much gravitated to it since the build times are so much better.

@Korijn
Copy link
Contributor Author

Korijn commented Jun 27, 2016

I've been considering that! Looks like conda-forge already has a ninja package so it should be relatively painless.

@jakirkham
Copy link
Member

Seemed to help AppVeyor. Travis CI and CircleCI appear to still have issues. Maybe we can try cotire ( conda-forge/staged-recipes#897 ). Might require a patch to get working.

@Korijn
Copy link
Contributor Author

Korijn commented Jun 28, 2016

@jakirkham I'm not terribly sure how to go about adding cotire to this PR. Would you mind doing it if I give you permissions on the fork?

@Korijn
Copy link
Contributor Author

Korijn commented Jun 29, 2016

  • AppVeyor now passes all configurations.
  • AppVeyor doesn't want to extend our build times (at least, not for free).
  • Have not heard back from Travis or Circle yet.

@jakirkham I have an odd suggestion: iterating over subsets of the configurations through two or three PRs. Is that acceptable, or do we really want to get below the build timeout?

@Korijn
Copy link
Contributor Author

Korijn commented Jun 29, 2016

By skipping py3.4 on unix only I was able to get AppVeyor and CircleCI to pass. Travis still times out though. I will skip OS X once more so we can reach all-green. Then we can open another PR to push out the remaining configurations. Agree @jakirkham ?

@Korijn
Copy link
Contributor Author

Korijn commented Jun 30, 2016

Well, once Travis finally responds to let us know that it skipped the build... everything should be green. Compared to the feedstock, this PR upgrades the build scripts to function cross-platform. Due to CI build time limits, it unfortunately only adds 6 Windows configurations (x86 & x64 for py2.7, 3.4 & 3.5) to the already available linux builds (x64 for py2.7 & 3.5).

I'd like to merge and open another PR where we can dig into optimizing the build even further, using things like cotire as suggested by @jakirkham.

@Korijn
Copy link
Contributor Author

Korijn commented Jun 30, 2016

@ocefpaf This TravisCI build could also use a restart... :)

@Korijn
Copy link
Contributor Author

Korijn commented Jun 30, 2016

There we go! Ready for merge.

We can then open another PR where we can squeeze out OS X and Linux 3.4 support.

@ocefpaf
Copy link
Member

ocefpaf commented Jun 30, 2016

@ocefpaf This TravisCI build could also use a restart... :)

@Korijn you should have rights to restart Travis-CI and merge things here. Let me know if that is not working.

@ccordoba12 ccordoba12 merged commit 765fabd into conda-forge:master Jun 30, 2016
@Korijn
Copy link
Contributor Author

Korijn commented Jun 30, 2016

I do have merge rights on github, but no rights for CI.

@ocefpaf
Copy link
Member

ocefpaf commented Jun 30, 2016

I do have merge rights on github, but no rights for CI.

Maybe you need to manually sync your Travis-CI. That did bite me in the past.

@jakirkham
Copy link
Member

jakirkham commented Jun 30, 2016

With Travis CI, you have to resync your account.

@ccordoba12
Copy link
Contributor

ccordoba12 commented Jun 30, 2016

Merged! @Korijn, great work making the Windows builds to work!

I also noticed that VTK builds its own copies of freetype, libpng and other libraries we have. Maybe there are CMake options to deactivate that?

@jakirkham
Copy link
Member

Oops, sorry, @ocefpaf, that showed up after the comment posted.

@jakirkham
Copy link
Member

jakirkham commented Jun 30, 2016

Sorry, I have not been keeping up-to-date here. Your incremental approach to support sounds totally reasonable to me @Korijn.

@jakirkham
Copy link
Member

@jakirkham I have an odd suggestion: iterating over subsets of the configurations through two or three PRs. Is that acceptable, or do we really want to get below the build timeout?

Could you please explain what you have in mind for this, @Korijn?

@jakirkham
Copy link
Member

It looks like CircleCI is parallellizing (there are 4 sub-builds) but they're all running the same steps. Shouldn't it split up over the different configurations? It gets py2.7 and py3.4 done, and runs out of time working on py3.5 currently.

Yeah, CircleCI doesn't have a build matrix like Travis CI does. 😞 Though it may be possible to use the parallelism feature that you were looking at to imitate this. See this discussion and this code. I'll put this in a conda-smithy issue so we can figure out if/how we can use this.

TravisCI jobs stop just before the test phase. :/ It might succeed with only 5 minutes more time.

😢 We have sometimes skipped some tests when this happens. Though I'd prefer to try cotire or perhaps try generating an XCode Project with CMake instead. I've never tried the latter, but maybe that affects the performance.

@jakirkham
Copy link
Member

I also noticed that VTK builds its own copies of freetype, libpng and other libraries we have. Maybe there are CMake options to deactivate that?

Good catch, @ccordoba12. Yeah, this must save some build time if we skip building these and use our own. What do you think, @Korijn?

@demarle
Copy link

demarle commented Jun 30, 2016

Yes, most of our TPLs have a VTK_USE_SYSTEM cmake option, please do try them. Of course, if there is a significant amount of drift between our version and whatever we find on the system they will not work.

@jakirkham
Copy link
Member

Is there somewhere in the source or docs we should look to see what versions are used by VTK, @demarle?

@Korijn
Copy link
Contributor Author

Korijn commented Jun 30, 2016

Something for the next PR then, I guess. :)

We're currently experiencing some troubles with offscreen rendering on headless servers using this package, and the docs seem to indicate that it requires a custom build, so I'll be exploring the options there as well in another PR. Having some trouble finding docs that are up to date though. E.g.:

@Korijn
Copy link
Contributor Author

Korijn commented Jun 30, 2016

CircleCI responded and recommended us to look into parallellism, so that resonates with your suggestions @jakirkham

@Korijn Korijn deleted the cross-platform branch July 1, 2016 06:16
@jakirkham
Copy link
Member

FYI I have disabled CircleCI parallelism on this feedstock as we don't have a way to leverage that currently. Hope that is ok.

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.

9 participants