Skip to content

Fix the msvc-build scripts (specifically those creating VS projects) for g4w-sdk / g4w #256

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

Closed
wants to merge 35 commits into from

Conversation

PhilipOakley
Copy link

This patch series fixes the msvc-build script and associated files in the compat and contrib directories to allow us Windows folks to use that most popular of IDEs, Visual Studio (despite it being from Microsoft...). The Git upstream (Junio) has asked that it be well tested here before it he could take it into his tree as he has no way of testing.

Reports are welcomed. Testing of the SAFESEH (top patch) option would be useful as I have no test system (at the moment) that brings out the conditions that need that case.

@PhilipOakley
Copy link
Author

Just wondering if anyone has had a chance to look at this within the new SDK (rather than the msysgit environment which was reported here-> msysgit#318).

Philip

@nalla
Copy link

nalla commented Jul 30, 2015

I gave it a try to compile it with VS2013. But failed so far due to missing knowledge in that area.

@dscho
Copy link
Member

dscho commented Jul 30, 2015

I planed to invest some time mid next week to test it and get it merged.

@YueLinHo
Copy link

@PhilipOakley
I did these:

  1. run git-sdk-64\git-cmd.exe
  2. cmd: cd usr\src\git
  3. cmd: msvc-build --vs
  4. git.sln is executed and updated to VS2013
  5. Build solution in VS2013 with lots of error.
  6. Check SAFESEH => still YES, instead of NO
  7. Change SAFESEH to NO manually (except the libgit, vcs-svn_lib, xdiff_lib projects)
  8. Build solution again, see error part 1, part 2, part 3

@YueLinHo
Copy link

second try:
cleanup first
run git-sdk-64\git-cmd.exe

D:\GfW\git-sdk-64>cd usr\src\git

D:\GfW\git-sdk-64\usr\src\git>make common-cmds.h
GIT_VERSION = 2.4.6.windows.1.17.g71fb0d9
    GEN common-cmds.h
.txt: No such file or directoryit-add
.txt: No such file or directoryit-bisect
.txt: No such file or directoryit-branch
.txt: No such file or directoryit-checkout
.txt: No such file or directoryit-clone
.txt: No such file or directoryit-commit
.txt: No such file or directoryit-diff
.txt: No such file or directoryit-fetch
.txt: No such file or directoryit-grep
.txt: No such file or directoryit-init
.txt: No such file or directoryit-log
.txt: No such file or directoryit-merge
.txt: No such file or directoryit-mv
.txt: No such file or directoryit-pull
.txt: No such file or directoryit-push
.txt: No such file or directoryit-rebase
.txt: No such file or directoryit-reset
.txt: No such file or directoryit-rm
.txt: No such file or directoryit-show
.txt: No such file or directoryit-status
.txt: No such file or directoryit-tag

any idea with these error?

@YueLinHo
Copy link

BTW, these 2 patterns could be also added into .gitignore if using VS2013.

*.opensdf
*.sdf

@PhilipOakley
Copy link
Author

@YueLinHo Thanks for the feedback.

Yes the common-cmds.h should be build first (a pre-requisite of a regular build).

  • The error message for that implies a display problem with a missing " g" after the 'No such file or directory', or some other issue with either the error reporting, or the way the filenames are picked up by the make file (which isn't part of my script ;-)

Yes, the command is 'msvc-build --vs', which should down load a couple of directories/repos if missing (as per the README), and then internally do a make-dryrun to create/generate the .sln and .vcproj files, which is where errors (hence patches) mainly were.

Finally it then does the little command dance to start up the VS20xx version.

I have had issues if I accidentally built the patch series on top of Junio's git/git because the setting weren't right (it still thought 'Ah Linux', rather than 'Oh Windows').

On the SafeSEH issue, that is one I haven't been able to test, and the VS GUI is incomplete in that it has two locations for SafeSEH, (a) the old option flag, and (b) the new xml flag which the gui displays. For backward compatibility I used the former, and to use it you should add SAFESEH=YesPlease to the script invocation, so that it is passed down the environments to the make-dry, and finally picked up as an integral part of the perl generation of the .sln & .vcproj files.

Don't know about the VS2013 issues yet. I've just received a new PC (Win 7 pro / 64-bit) for the house which I'm in the process of building and installing the family on... (sloow).

hope that gives a bit of help and background.

@YueLinHo
Copy link

@PhilipOakley Thank you. :)
One more question:
run both make common-cmds.h and msvc-build --vs via git-cmd.exe, instead of git-bash.exe?

@PhilipOakley
Copy link
Author

@YueLinHo - I've done my testing on the 'bash' window, rather than using the cmd.exe version (i.e. when I install, I say I'll keep them 'separate'), so there may be something there that needs fixed.

@dscho
Copy link
Member

dscho commented Aug 5, 2015

Yeah, I would definitely first try with Git Bash. In general, I am not sure whether it is worth fixing the build in cmd if that is broken.

@PhilipOakley
Copy link
Author

@dscho the first step is to prove/disprove that the cmd.ex (and the bash) properly run the make common-cmds.h part, which was the initial error report. Unfortunately I've had some family/home issues so haven't been on top of this in the last few weeks. I'm in process of 'building' a new Win7 64-bit PC, so hope to be able to test soon.

I'll keep chipping away at the issue. Thanks everyone for the feedback and support.

@@ -40,6 +41,7 @@ sub showUsage
# Parse command-line options
while (@ARGV) {
my $arg = shift @ARGV;
#print "Arg: $arg \n";

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@dscho
Copy link
Member

dscho commented Aug 11, 2015

So I spent some hours trying to get this to work with Visual Studio 2013. Here is my report before I call it a night: quite a couple of compile errors. I was able to put a couple of very dirty work arounds in, along with some legitimate changes: dscho@821cdad

# if cl.exe does not exist, populate vsvars with the most recent Visual Studio path
type cl.exe 2> /dev/null ||
vsvars="$(ls -t \
"$PROGRAMFILES/Microsoft Visual Studio"*/Common7/Tools/vsvars32.bat |

This comment was marked as off-topic.

This comment was marked as off-topic.

@dscho
Copy link
Member

dscho commented Aug 17, 2015

FYI I am preparing the release right now, and as this issue is not really crucial from a user's point of view, I will take it out of the milestone.

'make clean', or a 'git clean -dfx' will delete the PM stamp file,
so it cannot be a direct target in such clean conditions, resulting
in an error.

Normally the PM.stamp is recreated by the git/Makefile, except when
a dry-run is requested, for example, as used in the msysgit msvc-build
script which implements the compat/vcbuild/README using
contrib/buildsystems. The script msvc-build is introduced later in this
series.

Protect the PM.stamp target when the PM.stamp file does not exist,
allowing a Git 'Makefile -n' to succeed on a clean repo.

Signed-off-by: Philip Oakley <[email protected]>
---
This is development of the original "[PATCH 4/17] Makefile: a dry-run
can error out if no perl. Document the issue" 2015-06-25,
(http://marc.info/?l=git&m=143519054716960&w=2), which simply documented
the issue and then used NO_PERL to avoid the problem. See follow on
email thread for some discussion.
@PhilipOakley
Copy link
Author

I haven't looked at the perl generator, so I can't comment on that. But yes, in config.mak.uname there is MSVC_SDK10 quoted string and that is concatenated in the BASIC_CFLAGS variable and the quotes are arriving weird.
You might try removing the quotes on the MSVC_SDK10 line and adding around the values on the BASIC_CFLAGS lines and see if that helps.

For example,
MSVC_SDK10=C:/Program Files (x86)/Windows Kits/10
BASIC_CFLAGS=... -L"$(MSVC_SDK10)/Include/10.0.10240.0/ucrt" ...

and see if that goes thru better.

Thanks. I'd just about come to that conclusion after a lot of bingoggling (*) around make file space quoting issues.

I'll probably also need to make sure the "-I" options are processed properly as well, as it's not special in the generator.

(*) portmanteau of bing googling ;-)

The change to the quoting of the -I and -L options in the config.mak.uname has worked for the dry-run output, and partially works for the vcproj generate step.

I included adding a space after the option and before the dq quotation mark for the space-in-filename-paths.

The -L options in the generated vcproj are still wrong (lacking the quotes), so that's next.

@PhilipOakley
Copy link
Author

I'm getting suspicious that the -L flags such as -L"$(MSVC_SDK10)/Include/10.0.10240.0/ucrt" should be assigned into the LDFLAGS, not the CFLAGS.

e.g. https://github.com/dscho/git/blob/master/Makefile#L1016
vs https://github.com/dscho/git/blob/msvc/config.mak.uname#L421 (423, 425, 427).

Though it's more of a guess based on local code convention.

...

Yay, appears to get rid of the errors I was seeing in the 'additional options' of the command line properties.

Now (tomorrow) to sort the 'struct timespec' ifndef definition

@mfriedrich74
Copy link

mfriedrich74 commented Nov 2, 2016

-L specifies a linker library path, it us useful on CFLAGS only when cl.exe
is also used to link, which is rarely the case.
if cl.exe is told to link it will pass these options to link.exe.

@jeffhostetler
Copy link

jeffhostetler commented Nov 2, 2016

Yes, traditionally a "-L" arg should be in LDFLAGS. The script in "compat/vcbuild/scripts/clink.pl" just gets the full Unix cc/gcc command line and transforms it into a cl.exe or link.exe command line (after all the shell variables have been expanded). So when building with the Makefile it doesn't matter. If this is causing an issue for the generator, you could change it.

@PhilipOakley
Copy link
Author

Yes, traditionally a "-L" arg should be in LDFLAGS. The script in "compat/vcbuild/scripts/clink.pl" just gets the full Unix cc/gcc command line and transforms it into a cl.exe (after all the shell variables have been expanded). So when building with the Makefile it doesn't matter. If this is causing an issue for the generator, you could change it.

Thanks. I have now done the change (locally) and it now works as per the conversion script.

It's useful to have that subtle difference confirmed.

@jeffhostetler
Copy link

Just a thought. If you're building with vs2008, you won't have the Windows Kits directories -- I think they are new with vs2015.

You might open vs2008 developer command prompt and print the environment variables that the vcvars*.bat script sets up and compare that with the ones I put in the config.mak.uname. (I had to set all of them explicitly so that the Makefile would have them.)

@randomascii
Copy link

While -L should be a library path, the example from a few messages ago is not actually a library path, it is an include file path:

"$(MSVC_SDK10)/Include/10.0.10240.0/ucrt"

Therefore it should be going into a -I argument in CFLAGS. The Windows 10 SDK directory (default location, c:\Program Files (x86)\Windows Kits\10) contains Include and Lib directories. Those then contain variously numbered version directories Mine contains these three:

10.0.10150.0
10.0.10240.0
10.0.14393.0

However only the 14393 directory on my machine has the full set of header files. It's all quite messy, but if we can make git build either with the most recent Windows 10 SDK (currently 14393) or with all versions, that would be best.

@dscho
Copy link
Member

dscho commented Dec 22, 2016

@PhilipOakley As of v2.11.0.windows.1, the Visual Studio 2015 support is included, and one of my continuous build jobs publishes vs/master that is generated whenever master changes.

You will notice that both @jeffhostetler and I relied heavily on your work to get there, thank you so much!

Now, unfortunately there were a couple of incompatible changes, in particular with our move toward using nuget.exe to retrieve the dependencies instead of relying on the (outdated) binaries on repo.or.cz. Therefore, it will require a little work to get this here PR rebased on top of the current master. Do you want me to give you a hand, or do you think you got everything you need?

@PhilipOakley
Copy link
Author

PhilipOakley commented Dec 22, 2016 via email

@dscho
Copy link
Member

dscho commented Dec 23, 2016

Hi @dscho, I had made some progress, but as usual was lagging behind the pack. I have a commit for fetching the nuget packages in the V2 format I found the version of .Net that allowed using nuget on XP SP3.

@PhilipOakley awesome!

FWIW I obtained access to a Windows-7 VM with VS2008 and came up with a couple of patches myself: http://github.com/dscho/git/compare/git-for-windows:master...vs2008

It is far from complete, of course, I basically only got libgit to build (but nothing was linked yet), but that branch may have a couple of gold nuggets for you. Sorry for the latest commit (a "Work In Progress" one, smooshing together all kinds of changes I had not yet committed, to be split and weeded out), I just ran out of time before the holidays.

I fixed one of @jeffhostetler bits of good work that needed the variables defined at the start of a block (c89 style)

Right. I think I recreated those: 39f9089 and 35ebda5, right? Compiling with DEVELOPER=1 under GCC did not catch those issues, of course, as that code does not get compiled with GCC ;-)

I hope to rebase my old PR (you had already taken some of the commits...) in the next few days.

I look forward to that!

I have just seen that you say 8b018c7 that you have recreated some of the compatibility code for XP (..SP3), which is great news for me.

Good. I hoped it would make your life easier.

Thank you for all your good work.

The same goes for you! Thank you for keeping the ball rolling, and also for keeping an eye out for issues on the Git mailing list as well as helping to address the ever-increasing flood of user requests and tickets.

Happy holidays and a happy new year!

@PhilipOakley
Copy link
Author

Hi @dscho,
your fixes at : 39f9089 and 35ebda5, look right.

I have rebased my dscho-msvc branch on top of your VS2008.

The one big problem for compilation on XP that I've see in wrting up one of the TODO commits is that the necessary NuGet version (2.7.4) that support the 'restore' command has been removed from the NuGet.org downloads, so it is no linger possible for folks on XP to bridge that gap (a .Net version, a NuGet version, and 'restore'). I have a copy of NuGet V2.7.4..

For the XP/VS2008 version I still have to sort out the correct install of the WDK that holds 'NtQueryObject' symbol - i.e find the location so that it is automatically picked up by VS2008, rather than it being a manual user setup.

Also I've yet to work through your vcxproj.pm code that does the right things with libiconv, so I can get that into the old vcproj.pm

Overall the project now builds with only the "fatal error LNK1181: cannot open input file 'libiconv.lib'", though the is obviously an imprortant warning ;-)

@dscho
Copy link
Member

dscho commented Dec 27, 2016

That is great progress!

Just so I understand: newer nuget.exes do not support XP, or is there a different problem with them?

As to NtQueryObject(), I was under the impression that this function had to be accessed manually on XP, i.e. via DECLARE_PROC_ADDR/INIT_PROC_ADDR.

And yes, libiconv.lib is rather important. The real question is whether the NuGet package contains a version that is compatible with VS2008...

@PhilipOakley
Copy link
Author

PhilipOakley commented Jan 2, 2017 via email

@dscho
Copy link
Member

dscho commented Jan 2, 2017

Sorry for the delay. I've had man-flu (head cold).

Don't worry. Was down with a serious cold myself.

Correct. The last version that supported XP, and supported the 'restore' command, was 2.7.4. And it has disappeared from the NuGet repository, making life difficult for any others wanting to follow along.

Hrm.

But we actually do not need nuget.exe. It makes things convenient, for sure, but we do not have to have it. Take for example the package openssl.v120.windesktop.msvcstl.dyn.rt-dyn, as declared in compat/vcbuild/packages.config. You can easily download it manually via:

curl -O https://api.nuget.org/packages/openssl.v120.windesktop.msvcstl.dyn.rt-dyn.1.0.2.1.nupkg

Just one quirk there: the openssl.v120.windesktop.msvcstl.dyn.rt-dyn.x64 package violates the version number format convention and is only available as version 1.0.2 (i.e. not 1.0.2.0).

The thing to keep in mind after that is that .nupkg files are .zip files, really, with a certain directory layout (nuget.exe excludes some files from being unpacked). So you can even unpack them with unzip; we do not care about superfluous files, so you need not even bother with excluding the files that would have been excluded by nuget.exe.

I just hacked up a script that should work (only very lightly tested here):

#!/bin/sh

die () {
	echo "$*" >&2
	exit 1
}

for package in $(sed -n 's/.*id="\([^"]*\)" version="\([^"]*\).*/\1.\2/p' \
	< compat/vcbuild/packages.config)
do
	dir=compat/vcbuild/GEN.PKGS/$package

	nupkg=$dir/$package.nupkg
	test $nupkg ||
	curl -o $nupkg https://api.nuget.org/packages/$package.nupkg ||
	curl -o $nupkg https://api.nuget.org/packages/${package%.0}.nupkg ||
	die "Could not download $nupkg"

	test -f $dir/_rels/.rels ||
	(cd $dir && unzip $package.nupkg) ||
	die "Could not unpack $nupkg"
done

If you run that in the top-level directory, it should populate the compat/vcbuild/GEN.PKGS/ directory structure. (It actually overpopulates them, extracting more than nuget.exe would have, as I pointed out above, but that does not matter, really, as you can simply ignore the extra files.)

Ensuring that the script converted the lib name correctly when processing the dry-run output - you have some code in the VS2015 script that does some fancy stuff.

Yep. But you can copy that stuff back from Vcxproj.pm to Vcproj.pm (I copy-edited the latter to come up with the former, after all.)

Second, in my install, the nugget download is named iconv (rather than libiconv), so I wasn't sure if I had done stuff right. At this stage, I haven't pointed the project solution at the download (in build-extras), so it won't be seen anyway.

I do see an iconv.lib but no corresponding iconv.dll here, but that may be an artifact of my build process.

@PhilipOakley
Copy link
Author

@dscho, That info about the Nuget pacakges and the workaround on how to decode them is very useful. It means I can stop worrying about that part.

For my own download of the NuGet pakages (back in late October!) I had used the V2 api portal (covered by a small patch).

In terms of the iconv I may have been confused...

I saw in my C:\git-sdk-32\usr\src\git\compat\vcbuild\GEN.DEPS\lib (note it's the GEN.DEPS) that it had the iconv.lib, but not a libiconv.lib, hence my concerns.

However, after your note, a bit more looking showed (in the GEN.PKGS) that I have:
"C:\git-sdk-32\usr\src\git\compat\vcbuild\GEN.PKGS\libiconv.1.14.0.11\build\native\lib\v100\Win32\Release\static\cdecl\libiconv.lib", and the variants (debug/release; static/dynamic; V100,V110).

And the same for the redistributables
"C:\git-sdk-32\usr\src\git\compat\vcbuild\GEN.PKGS\libiconv.redist.1.14.0.11\build\native\bin\v100\Win32\Debug\dynamic\cdecl\libiconv.dll"

So I do have the libiconv - I just wasn't looking in the right place, and haven't yet setup the .sln to link to them. Shaking of the head/chest cold has taken longer than I'd hoped - the next step (wark aside) is to repivot your code to get the linkages automatically correct, and then fingers crossed...

As an aside, excusing my ignorance, what is the difference between V110 and V100 ?

@jeffhostetler
Copy link

jeffhostetler commented Jan 4, 2017

v100 and v110 refer to the version of Visual a Studio.
v100 is for VS 2010.
V110 is for VS 2012.

The following link is unrelated to the discussion here, but talks a little about the versions:
https://social.msdn.microsoft.com/Forums/vstudio/en-US/e4c7a416-8c34-40c6-b3c3-d08be7515010/build-tools-unavailable?forum=vssetup

@dscho
Copy link
Member

dscho commented Jan 4, 2017

v100 is for VS 2010.
V110 is for VS 2012.

Ouch. That may very well mean that there is no support for VS2008 in any of the NuGet packages?

@PhilipOakley
Copy link
Author

v100 is for VS 2010.
V110 is for VS 2012.

Ouch. That may very well mean that there is no support for VS2008 in any of the NuGet packages?

I was going to guess that the difference (beyond the VS versions) was the difference in the 'C' Run Time (CRT) that has been mentioned else where, and which one (of the CRT's) that each VS version auto linked to.

Otherwise, why do the different VS versions need different libraries - I would have thought that the library format was version independent..

@mfriedrich74
Copy link

mfriedrich74 commented Jan 4, 2017

You can mix libraries from different VS versions.
Appart from higher memory footprint, you will have to make sure a few things.

This is what I remember, might be incomplete:

  • new and delete on an object is performed by the same runtime
  • avoid STL use in shared headers

General these things must be the same setting accross all libs and own code to link successfully

  • character encoding setting (utf-16 vs ANSI vs none) and macro
  • static/dynamic crt linkage
  • debug/release settings and macros
  • 32/64 bitness obviously

@randomascii
Copy link

randomascii commented Jan 4, 2017 via email

@mfriedrich74
Copy link

Windows Embedded Standard 2009 still gets patched (end of license is 2024), that is still an XP.

@PhilipOakley
Copy link
Author

PhilipOakley commented Jan 4, 2017 via email

@dscho
Copy link
Member

dscho commented Jan 5, 2017

Is support for VS 2008 really desirable?

Hi @randomascii ! Thank you for sharing your thoughts with us. I am sure you have thought things through and decided for good reasons that you do not want to work on VS 2008 support. Others, such as @PhilipOakley have equally good reasons to do work on VS 2008/XP support.

I welcome all contributions, whether they target obsolete or not so obsolete platforms, as long as they do not increase my maintenance load (which they don't, I simply do not work too much on VS 2008 and focus exclusively on VS 2015 myself).

Hopefully that clarifies the situation?

@dscho
Copy link
Member

dscho commented Feb 21, 2017

@PhilipOakley how about a force-push with your current state?

@PhilipOakley
Copy link
Author

PhilipOakley commented Feb 23, 2017 via email

@dscho
Copy link
Member

dscho commented Feb 23, 2017

I'll add the split files and a big commit note as to where I was, but it may be a day or so to get some clear space.

No worries! It will be good to have the work-in-progress state in the open, though, so that other developers can possibly step in and try to come up with improvements.

@PhilipOakley
Copy link
Author

I have force pushed my status. I included a top WIP commit with a summary.

The branch was rebased on to of @dscho 's repo.

There are two independant things I was hooked up on.

  1. Merge the generator scriptlets in line with dscho's example (more than two screenfulls to merg in one's head..)

  2. Is the Nuget version for XP that handles the 'restore' command, which was 2.7.4, still available somewhere via Microsoft? Do @dscho @jeffhostetler have any contacts who could clarify?

point 2 is not essential for VS2008, it only matters for those on older platforms that can't handle the new VS versions.

@dscho
Copy link
Member

dscho commented Sep 15, 2017

@PhilipOakley do you want to keep this ticket open?

@PhilipOakley
Copy link
Author

Let's close it. I know where it is. I can always re-open it if I manage to get the time to address this, (and I'm still on the same hardware ;-)

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.

7 participants