Fix AppVeyor build failures due to CRTDBG linking issue #4347
Conversation
|
It does not fix the issue. sigh |
|
Ugh. This is actually a real issue introduced into our CMake build instructions. As soon as |
|
I've fixed the issue. Please don't ask how it could've been working previous to this commit, I really have no idea. I also don't think that the CMakeLists.txt split can be at issue here, instead I'd bet that this has changed due to an update of CMake at AppVeyor. Even though the first commit is not required anymore, I'd still like to retain it. First, having explicit mention of the build image is nice. And second, updating our toolchain is probably also worthwhile. @ethomson: Could you please merge this ASAP (that is, after CI runs through and you are happy with these commits) to fix our AppVeyor builds? |
|
Do we specify a minimum version of VS we support? Would we want to request the earliest supported version in AppVeyor so we don't accidentally rely on new features? |
pks-t
changed the title
|
We could also have three different build jobs for VS12, VS14 and VS15. VS11 is also installed on the VS12 images, but it feels a little bit awkward to make use of it when it's officially a build image for VS12. |
When the MSVC_CRTDBG option is set by the developer, we will link in the
dbghelper library to enable memory lead detection in MSVC projects. We
are doing so by adding it to the variable `CMAKE_C_STANDARD_LIBRARIES`,
so that it is linked for every library and executable built by CMake.
But this causes our builds to fail with a linker error:
```
LINK: fatal error LNK1104: cannot open file 'advapi32.lib;Dbghelp.lib'
```
The issue here is that we are treating the variable as if it were an
array of libraries by setting it via the following command:
```
SET(CMAKE_C_STANDARD_LIBRARIES "${CMAKE_C_STANDARD_LIBRARIES}"
"Dbghelp.lib")
```
The generated build commands will then simply stringify the variable,
concatenating all the contained libraries with a ";". This causes the
observed linking failure.
To fix the issue, we should just treat the variabable as a simple
string. So instead of adding multiple members, we just add the
"Dbghelp.lib" library to the existing string, separated by a space
character.
pks-t
force-pushed
the
pks/appveyor-vs2015
branch
from
e792822
to
7a2db5a
Compare
|
I've just done so. Instead of building for all different configurations, this simply retains jobs for the oldest possible configuration, Visual Studio 11 2010, and adds two more current jobs based on Visual Studio 14 2015. |
AppVeyor currently does provide three standard build worker images with VS2013, VS2015 and VS2017. Right now, we are using the implicitly, which is the VS2015 one. We want to be more explicit about this, so that we can easily switch build images based on the job. So starting from this commit, we explicitly set the `APPVEYOR_BUILD_WORKER_IMAGE` variable per job, which enables us to choose different images. To be able to test a wider range of build configurations, this commit also switches the jobs for VC2010 over to use the older, VS2013 based images. As the next commit will introduce two new jobs for building with VS2015, we have then covered both build environments. Also, let us be a bit more explicit regarding the CMake generator. Instead of only saying "Visual Studio 10", use the more descriptive value "Visual Studio 10 2010" to at least avoid some confusion surrounding the versioning scheme of Visual Studio.
In order to cover a wider range of build environments, add two more jobs which build and test libgit2 on Visual Studio 14 2015.
pks-t
force-pushed
the
pks/appveyor-vs2015
branch
from
7a2db5a
to
03a95bc
Compare
|
I meant it more in the sense that if the oldest image they have is for 2012, maybe we should be using that. Lately the macOS builds are the slow ones, so I guess building with more on Windows doesn't hurt that much, though. |
|
Yeah, I understood you all right :) But I guess it's still superior to just have different environments here. Especially as I've noticed the same as you, with AppVeyor builds being faster than Travis builds right now. |
|
Hmm. I'm a bit concerned about adding more Windows runners, since we share them with LibGit2Sharp. There are a lot of options if that's a problem though. I'll merge this but we should keep an eye on usage and figure out how to proceed. |
|
Okay, good to know. What other options are you referring to? |
|
The two things that came immediately to kind are that we could pay for more parallel jobs or move LibGit2Sharp out of the libgit2 org. Not super in love with either but there's not a ton of work going on in LibGit2Sharp right now so it's not very high priority, imho. |
|
Well, as "pay for more" effectively means that we (or, probably, you) need to do so from our own pockets, I'm not too keen on that one. I guess just observing how we do with those additional jobs is okay for now. We can still drop them later on again. |
|
Dear lord. Now that AppVeyor is fixed, Travis is hanging again. This is so annoying... |
|
Thanks for taking care of this @pks-t! |
AppVeyor currently does provide three standard build worker images with
VS2013, VS2015 and VS2017. Right now though, we use the CMake generator
for Visual Studio 11 2012, which is not an official part of any of the
build images. It seems like this is actually biting us right now, were
our recent builds are failing due to linker errors.
While it is not really clear what is causing the actual problem,
explicitly configuring the target build platform seems like the right
thing to do. To do so, we now explicitly request the VS2015 build image.
This also requires us to bump the CMake generator to use the same
version.
I hope this fixes our recent issues. While this does not fix the actual problems that we're seeing, I guess updating to an "officially supported version" is the right thing to do. And honestly, I'm just much too annoyed right now by all those awful CI issues we are and were having in the last few months.