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

Support for VS2019 msbuild #11

Merged
merged 1 commit into from
May 27, 2019
Merged

Support for VS2019 msbuild #11

merged 1 commit into from
May 27, 2019

Conversation

gerhardol
Copy link
Contributor

Fixes #10
Support for VS2019, where msbuild path has changed

frontend.bat Outdated Show resolved Hide resolved
frontend.bat Outdated Show resolved Hide resolved
frontend.bat Outdated Show resolved Hide resolved
@gerhardol
Copy link
Contributor Author

Used in gitextensions/gitextensions#6500, only VS2017 available in AppVeyor yet
Tested locally with VS2019 and suppressed 2019 to pickup 2017 instead.
I do not understand why the '-nologo' error message appears and what fixes it...

@3F
Copy link
Owner

3F commented Apr 24, 2019

Thanks for your contribution!

I do not understand why the '-nologo' error message appears and what fixes it...
Without mentioning vswbin (a4 in generation), the variable is never substituted:

an amazing batch-engine :(

I think, this is the same problem like it was for this block. Part of this uses evaluation by force, like:

set /a "idx+=1" & call :eval arg[!idx!] v
.........................^...\___^___/->

exactly because of bug similar to processing with GEQ operator, below:

set /a "idx+=1" & if %idx% LSS !amax! goto loopargs
.......................^....^

My idea for the mentioned an evaluation by force was through passing arguments to function, like ping-pong:

set _vl=!%1!
...
set %2=!_vl!

looks like we can try to use something similar or also call :eval. But today's :dbgprint will not display more than 3 args:

:dbgprint {in:str} [{in:uneval1}, [{in:uneval2}]]
:: full argument processing requires more code, therefore I'm considering only first two after incoming string

hmm, any idea ?

seems I also need to test this later in real usage :( today or tomorrow, I hope.

Btw, functional tests can be raised by tests.bat

frontend.bat Outdated Show resolved Hide resolved
frontend.bat Outdated Show resolved Hide resolved
frontend.bat Outdated Show resolved Hide resolved
@gerhardol
Copy link
Contributor Author

Updated, fails for other reason now in AppVeyor:
https://ci.appveyor.com/project/gitextensions/gitextensions/builds/24135726

For tests, I assume thatoptions to set preferred msbuild version will be needed, but how to test this without requiring specific installs?

@3F
Copy link
Owner

3F commented Apr 27, 2019

For tests, I assume thatoptions to set preferred msbuild version will be needed, but how to test this without requiring specific installs?

Here, I'm using this batch script test-cases through msbuild targets

This is why hMSBuild also contains an post-action block, like there:

Or what do you mean?

Build.cmd : 'val2}]]' is not recognized as an internal or external command,

Looks like it does not related to hMSBuild because, as I see, you're using compiled hMSBuild.full.bat script. I mean, this 'val2}]]' data shouldn't be generated from us in any case.

But I can also inspect your build later If you sure with this.

By the way, why not to use something like:

set msbuild=hMSBuild -notamd64

instead of:

for /f "tokens=*" %%i in ('hMSBuild.bat -only-path -notamd64') do set msbuild="%%i"

from your Build.cmd

@3F
Copy link
Owner

3F commented May 23, 2019

So, before merge this PR, @gerhardol and anyone else:

Do you have some new noticed strange behavior or something more for this changes? vs2019 and old versions too.

We need to consider this before 28 May. Thanks.

@3F 3F added this to the v2.1 milestone May 23, 2019
@gerhardol gerhardol force-pushed the bugfix/fix-10-vs2019 branch from 2714c39 to ead01cc Compare May 25, 2019 21:50
@gerhardol
Copy link
Contributor Author

I fixed a whitespace change
However, when running the script in AppVeyor it fails with the errors I mentioned earlie with the full version.
The compiled version has no errors but the path is empty.

I believe this PR can be merged (I have not added tests), either you squash it or I do it.
However, I start to give up using hMSbuild

By the way, why not to use something like:
set msbuild=hMSBuild -notamd64

It was set up the original way so I could easily see what path that was choosen.
I have changed it, still fails.
I have changed the way the script is used too

@gerhardol
Copy link
Contributor Author

To run on the Appveyor server, adding the following debug printouts get it working:
image

@3F
Copy link
Owner

3F commented May 25, 2019

@gerhardol

Wait a second, this behavior reminds me this: #2

Please check this out!

I think this is it. Because of adding additional irrelevant instructions before problem place. Either this or :eval problem that was described above o_O

either you squash it or I do it

I had plan to squash this of course. But you can do this yourself if you want normal merge. As you prefer.

@gerhardol gerhardol force-pushed the bugfix/fix-10-vs2019 branch from ead01cc to fe8bd4f Compare May 25, 2019 22:45
gerhardol added a commit to gerhardol/gitextensions that referenced this pull request May 26, 2019
Updated hMSBuild with patch 3F/hMSBuild#11
Use 64bit build where possible
Note: Using hMSBuild.full.bat gives parse errors on Appveyor why
the compressed/compiled version is used
@gerhardol
Copy link
Contributor Author

It is kind of working for me now.
I have to add the extra :dbgprint from the screenshot.
This is due to the :eval problem (attributes were not set but happened to work).

Can I add this printout to this PR or is there a better solution?
(I have patched my own PR right now).

I would have preferred to use the full version, as the compiled/compressed is impossible to debug.
However, the parse errors seen in AppVeyor only makes this impossible.

Thanks for the product, sorry that I get frustrated when the work does not play together with other parts of the puzzle...

@3F
Copy link
Owner

3F commented May 26, 2019

They are both compiled via vsSBE together with integrated GetNuTool core. Compression/minimization just provides more verification, controls, etc. As you already noticed:

Using hMSBuild.full.bat gives parse errors on Appveyor why the compressed/compiled version is used

However, should we add both versions for public releases? For batch scripts this is hard to achieve the same result, thus a single public version is better at least for support.

Can I add this printout to this PR or is there a better solution?

Let's see, I will try to debug this problem today (appveyor + same your commits). Because this can hide more critical problems such as #9 !

parts of the puzzle...

exactly my thoughts -_-

@gerhardol
Copy link
Contributor Author

However, should we add both versions for public releases?

If "full" has apparent problems (size?) then it should not be released. But I hope it is just something I have missed.
But I would prefer to add something that can be easily debugged and patched. #10 already have a reference to someone making a private patch because they did not want to deal with the changes.

@gerhardol gerhardol changed the title WIP Support for VS2019 msbuild Support for VS2019 msbuild May 26, 2019
@3F
Copy link
Owner

3F commented May 26, 2019

Okay... Firstly, I've reproduced https://ci.appveyor.com/project/gitextensions/gitextensions/builds/24135726 error locally:

D:\tmp\_Issues\hMSBuild\PR11\gitextensions-572527c8e46e864112c5328a4ef36a2686ffb72c>Setup\Build.cmd
'val2}]]' is not recognized as an internal or external command,
operable program or batch file.
The filename, directory name, or volume label syntax is incorrect.
Checking for updates from https://www.nuget.org/api/v2/.
...

Now I'm sure this is CRLF problems (again see #2)
gerhardol/gitextensions@572527c

Secondly, I checked new commits:

https://ci.appveyor.com/project/gerhardol/gitextensions/builds/24819082
gerhardol/gitextensions@6921f49

See:

'l2}]]' is not recognized as an internal or external command,
operable program or batch file.
The filename, directory name, or volume label syntax is incorrect.

Can you try again with full version. I don't see problems at least for your 6921f49108dc6a

You can also try to use git config --global core.autocrlf true if git attr does not help.
Let me know if you have questions

@3F
Copy link
Owner

3F commented May 26, 2019

But I would prefer to add something that can be easily debugged

I see. My usual debugging includes recompilation that's hard for end-users because you need to clone repo and prepare environment for diag.

I'll think about both public versions, you can create issue to consider this later.

gerhardol added a commit to gerhardol/gitextensions that referenced this pull request May 26, 2019
Updated hMSBuild with patch 3F/hMSBuild#11
Use 64bit build where possible
Set filemode to crlf
Note: Using hMSBuild.full.bat gives parse errors on Appveyor why
the compressed/compiled version is used
gerhardol added a commit to gerhardol/gitextensions that referenced this pull request May 26, 2019
Updated hMSBuild with patch 3F/hMSBuild#11
Use 64bit build where possible
Set filemode to crlf
Note: Using hMSBuild.full.bat gives parse errors on Appveyor why
the compressed/compiled version is used
@gerhardol
Copy link
Contributor Author

Can you try again with full version

Tested, executes also without the ":dbgprint" patch
(attributes changed already - thought they were OK before my changes).

So all seem fine (even if tests are missing).
For releasing .full or not: I prefer that, will not protest if you choose to release one version.

Thanks!

@3F 3F merged commit d3c2970 into 3F:master May 27, 2019
@3F
Copy link
Owner

3F commented May 27, 2019

Accepted.

@gerhardol Thank you again for your contribution. Planned v2.1 will not contain an .full version. However, I'll think about adding this in future releases.

3F added a commit that referenced this pull request May 27, 2019
* NEW: Support for VS2019 msbuild (Thanks @gerhardol)

* CHANGED: Removed possible last extra slash `\` from path, ie. ~ `..\Bin\\MSBuild.exe`
           When x32 for searching from Visual Studio with `-notamd64` key.
           Part of PR #11

* CHANGED: Default remote vswhere is 2.6.7.
           https://github.com/microsoft/vswhere/releases/tag/2.6.7
@gerhardol gerhardol deleted the bugfix/fix-10-vs2019 branch May 27, 2019 17:23
3F added a commit that referenced this pull request Jan 17, 2020
https://github.com/3F/GetNuTool/releases/tag/1.8

This also updates build scripts because of modern vsSolutionBuildEvent 1.14 (used from GetNuTool 1.8)
+known problem: 3F/vsSolutionBuildEvent#61

And as I promised, an compiled.full version now will be distributed together with official hMSBuild releases (zip package):
#11 (comment)
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.

MSBuild team is changing "{major}.0" to "Current"
2 participants