Let the MSVC build also be tested in the Azure Pipeline

[dscho]

This ensures that our MSVC build becomes a first-class citizen.

[AraHaan]

@dscho I am also still trying to figure out the pipeline for my C# projects, do you mind somehow helping me (hopefully based on my appveyor configurations in a way).

Also so I can have the status badges for current (including pr builds)/master branch (excluding pr builds) for both stable vs2017 (since vs2017 preview is no longer supported my msft) and vs2019 preview.

[dscho]

I am also still trying to figure out the pipeline for my C# projects, do you mind somehow helping me (hopefully based on my appveyor configurations in a way).

@AraHaan can you point me to any work-in-progress?

Also so I can have the status badges for current (including pr builds)/master branch (excluding pr builds) for both stable vs2017 (since vs2017 preview is no longer supported my msft) and vs2019 preview.

Yep, that makes a total lot of sense.

[AraHaan]

sure, for starters there is my https://github.com/AraHaan/XmlAbstraction, and various other projects at https://github.com/Elskom/

Basically this is what I would like:

1 VS2017 (Stable) job with .NET Core 3 preview 3 or newer & .NET framework 4.8
1 VS2019 (Stable) job with the above.
1 VS2019 (preview) job with the above.
And optionally a job that runs the tests separately.

However I forgot if there is only 1 free job though for open source so.

[dscho]

However I forgot if there is only 1 free job though for open source so.

There are up to 10 parallel jobs for free, for open source projects.

there is my https://github.com/AraHaan/XmlAbstraction, and various other projects at https://github.com/Elskom/

Have you tried to simply install the Azure Pipelines GitHub on these repositories? From what I can tell, it has a pretty neat "onboarding" experience, as it analyzes your code and suggests a template for the most common project types (e.g. a .NET Core project, such as yours).

Basically this is what I would like:

1 VS2017 (Stable) job with .NET Core 3 preview 3 or newer & .NET framework 4.8
1 VS2019 (Stable) job with the above.
1 VS2019 (preview) job with the above.
And optionally a job that runs the tests separately.

Please note that I am only using VS 2017 here. VS 2019 was just announced, so I do not know off-hand how quickly there will be support for it on Azure Pipelines.

[PhilipOakley]

@dscho regarding the review request: Could you clarify which parts I should review, so I don't do the wrong thing.

Is it:

1. review your 'teaching of the azure pipeline' (I can read through but am not familiar with Azure)
2. review an output/build product of the Azure pipeline (if so, can I get a pointer to the appropriate build product)
3. see if the .sln generation code still works for VS2017, at least as far as using the intelli-sense tracking of the code (I'd need to refresh my memory, and make sure I've got the right VS build, but it should be doable)
4. try the make MSVC=1 stuff, though I've forgotten (i.e. I can't remember if I knew how) how to progress to installed code (and hence testing/using).
5. you meant something else, and I've just misread the request.

At the moment I'm going with just #.3 and hopefully I can get to it during the weekend.

At present I'm going through the long 'get >4GB working', predicated on my 'creating a merging-rebase' release of the potential upstream fixes, predicated on resolving the normal conflicts (currently lots on the stash code), [perhaps predicated on me working out how to create a rerere data base from the most recent merging-rebase]. So at the moment I'm tripping over my own failures... :sigh:

Hopefully it's just #.3, and I can have a look. Thanks.

[dscho]

@PhilipOakley I meant really just looking over the patches and commit messages, hoping that you would spot any glaring bugs if there were any.

But yeah, the 4GB stuff is important, too.

[jeffhostetler]

Looks good. Thanks for digging (so deeply) into all of this!

[jeffhostetler]

Look good. Thanks!

[PhilipOakley]

Not sure the best way to review the series via the GH interface, so here's a few comments from using gitk -100 dscho-git/azure-pipelines-msvc & locally from a fresh fetch :

Summary: Good. only minor teaks at your leisure.

0b45281, ad1b2d43c1d8f, OK

404a1517d54d27 "msvc: avoid using minus operator on unsigned types" I'm guessing upstream may not like the backward 'const before var' style (in 'their' code), but it works. Also the align_padding_size() tweak isn't explained.

2596c55bb883f2 "fscache: avoid unnamed struct to avoid compiler warning" Somebody will want a better name than just 'u' 😉

f0fb7ad7eba8d "msvc: ignore some libraries when linking" reason looks sensible. I just haven't understood [ignorance] the nifty code (does it remove all the "l..." named libraries?). Maybe the "let's just ignore the rest" need more emphasis, or that the necessary ones are truly handled elsewhere. It's a sub-clause of long sentence late in the explanation.

95f6fa9d30e7 "msvc: handle DEVELOPER=1" Wow, good long list of ['our'] code violations with useful comments for future readers.

dc41989c91 "msvc: work around a bug in GetEnvironmentVariable()" That must have taken some debugging. good catch. nice message.

aa51c108ee5e "ci: really use shallow clones on Azure Pipelines" Good catch.

ff69984a2adae4ea "ci: also test with MS Visual C on Azure Pipelines" Neat. I just need to (re-)learn how to do that stuff manually on my local machine 😉

[PhilipOakley]

I just tested the .sln created by '5eafd06a83 Generate Visual Studio solution' and it loaded into VS2017 (with 'build 140' added) and compiled clean. ! ! ! (all fresh install/download/checkout). So apart from needing that build 140 within VS2017, its a great success!

I noted that 5eaf commit when comparing the git range-diff 7dcd3d86d469..2481c4cbe949856f eb2b3f5e4d595cd..5eafd06a83 which is between the "starting merging-rebase" for v2.21.0, and the similar merging rebase from shears/master to vs/master. The two range diffs were identical (254 patches) other than the extra 5eaf on vs/master (I think I picked that up by accident rather than shears/master).

As an aside, the range-diff doesn't appear to include the side branches that a shears merging-rebase todo list generates ~500+ patches (I'd got to 438/580 before aborting my trial rebase for lack of knowing the conflict resolutions - and rerere-train doesn't help - a possible student project etc ...)

[AraHaan]

wait why v140 instead of v141 in 2017 as a conditional thing on the projects like from within a *props file imported by it.

So then if one wants they can also manually add vs2019 there as well with v142.

[PhilipOakley]

v140 instead of v141 in 2017

Our starting point is/was way back when VS 2008 was still being used (at least where I worked), and the key part was to generate the .sln file that was used back then (on top of individual .vcpjoj files, without the 'x').

As best I understand it (I'm now a retired engineer, so software is just a component ... 😉) the newer VS 2017 method of working doesn't use the old .sln mechanism, unless you install (tick the box for) that legacy build 140 capability. I certainly got that warning message on loading the .sln after downloading the VS2017 from the net in the last few days (i'd guessed what to tick). So I then searched around regarding the Build 140 to confirm that I needed it and how to 'rebuild/modify/upgrade' to get that legacy build. [dscho has done the proper hard work though]

In summary, I believe it is because we create a legacy .sln project, that covers all the 19 'git' executables.

19>test-tool.vcxproj -> ..\test-tool.pdb (Full PDB)
========== Build: 19 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========

Obviously, we welcome contributions that help move the (Git for Windows) project forward to get better interaction with those who are wholly committed to the MS tool chain (It should be about the people not the tools...)

send more details about these *props files of which you speak.. (yours, in ignorance..)

[jeffhostetler]

WRT 140 vs 141 vs 142. It's just a matter of making a priority to update.

We managed to get Git to build using VS2015 using 140 using "make MSVC=1".
And then later with VS2017 using 140 and vcpkg and VsWhere using "make MSVC=1".
And then later we updated the build scripts to synthesize the various .vcproj/.sln files from the Makefile.

Updating the scripts/Makefile to select 141 or 142 or to update the synthesized .vcproj/.sln to
handle VS2019, shouldn't be that difficult. It's just something we haven't taken time to do.

[AraHaan]

Sorry, been busy working a lot that I did not get to reply for a few days. Hopefully I am off tomorrow though.

[dscho]

@PhilipOakley & @jeffhostetler thank you so much for your reviews!

Not sure the best way to review the series via the GH interface

The best would probably be to switch to the "Files changed" tab and to select individual commits, or to switch to the Commit tab and select one there, then hover over the lines in question and then click the white-on-blue plus button to add a comment inline.

404a151 "msvc: avoid using minus operator on unsigned types" I'm guessing upstream may not like the backward 'const before var' style (in 'their' code), but it works.

What "const before var" style?

Also the align_padding_size() tweak isn't explained.

True. I added this to the commit message:

-- snip --
Note that the change in the estimate_cache_size() function is needed because MSVC considers the "return type" of the sizeof() operator to be size_t, i.e. unsigned, and therefore it cannot be negated using the unary minus operator.

Even worse, that arithmetic is doing extra work, in vain. We want to calculate the entry extra cache size as the difference between the size of the cache_entry structure minus the size of the
ondisk_cache_entry structure, padded to the appropriate alignment boundary.

To that end, we start by assigning that difference to the per_entry variable, and then abuse the len parameter of the align_padding_size() macro to take the negative size of the ondisk
entry size. Essentially, we try to avoid passing the already calculated difference to that macro by passing the operands of that difference instead, when the macro expects operands of an addition:

	#define align_padding_size(size, len) \
		((size + (len) + 8) & ~7) - (size + len)

Currently, we pass A and -B to that macro instead of passing A - B and 0, where A - B is already stored in the per_entry variable, ready to be used.

This is neither necessary, nor intuitive. Let's fix this, and have code that is both easier to read and that also does not trigger MSVC's warning.
-- snap --

2596c55 "fscache: avoid unnamed struct to avoid compiler warning" Somebody will want a better name than just 'u' 😉

There is precedent for this, though: https://github.com/git/git/blob/v2.21.0/grep.h#L125-L132

f0fb7ad "msvc: ignore some libraries when linking" reason looks sensible. I just haven't understood [ignorance] the nifty code (does it remove all the "l..." named libraries?). Maybe the "let's just ignore the rest" need more emphasis, or that the necessary ones are truly handled elsewhere. It's a sub-clause of long sentence late in the explanation.

Yeah, my tiredness shows, eh?

I replaced the last paragraph in that commit by this:

-- snip --
We already handle the direct dependencies, e.g. -liconv is already handled as adding libiconv.lib to the list of libraries to link against.

Let's just ignore the remaining -l* options so that MSVC does not have to warn us that it ignored e.g. the /lnghttp2 option. We do that by extending the clause that already "eats" the -R* options to also eat the -l* options.
-- snap --

[dscho]

dc41989 "msvc: work around a bug in GetEnvironmentVariable()" That must have taken some debugging. good catch. nice message.

Yes. One entire work day.

I just tested the .sln created by '5eafd06a83 Generate Visual Studio solution' and it loaded into VS2017 (with 'build 140' added) and compiled clean.

That does not contain the fixes of this here PR, though...

As an aside, the range-diff doesn't appear to include the side branches that a shears merging-rebase todo list generates ~500+ patches (I'd got to 438/580 before aborting my trial rebase for lack of knowing the conflict resolutions - and rerere-train doesn't help - a possible student project etc ...)

Could I ask you to clarify? I am a bit confused about this statement...

wait why v140 instead of v141 in 2017 as a conditional thing on the projects like from within a *props file imported by it.

So then if one wants they can also manually add vs2019 there as well with v142.

@AraHaan I have no idea how to do that. Care to enlighten me?

WRT 140 vs 141 vs 142. It's just a matter of making a priority to update.

We managed to get Git to build using VS2015 using 140 using "make MSVC=1".
And then later with VS2017 using 140 and vcpkg and VsWhere using "make MSVC=1".
And then later we updated the build scripts to synthesize the various .vcproj/.sln files from the Makefile.

Updating the scripts/Makefile to select 141 or 142 or to update the synthesized .vcproj/.sln to
handle VS2019, shouldn't be that difficult. It's just something we haven't taken time to do.

Ah, I think I understand the conundrum now...

Yes, the MSVC=1 build will always be a bit trickier, and I will recommend to use Visual Studio instead (but this requires that extra commit on top of master that is force-pushed regularly to vs/master)...

So for the moment, my priority really is to get the source code to compile in the Azure Pipeline using MSVC, and to run the test suite there, all integrated into a neat single CI experience.