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

Use Visual Studio 2017 setup API to detect it in MsBuild module #2077

Merged
merged 3 commits into from
Sep 22, 2018

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Sep 5, 2018

Use my VsWhere library to detect Visual Studio 2017 in MSBuild module.

It's the same thing as #1752 but it uses the COM API directly instead of the executable.

One important note is that NuGet is used instead of paket as the dependency can't be pulled on netstandard1.6 and must be conditional (They had removed most of COM)

Due to the netstandard1.6 strangeness I don't know if you'll take the PR but seeing the other one languish I wanted to propose an alternative

@matthid
Copy link
Member

matthid commented Sep 5, 2018

So the reference/package itself works with paket, only the condition is the problem, correct?

@matthid
Copy link
Member

matthid commented Sep 5, 2018

Also have you checked that it actually works on netcore? Only because the APIs are back doesn't mean they are working :).

@vbfox
Copy link
Contributor Author

vbfox commented Sep 5, 2018

So the reference/package itself works with paket, only the condition is the problem, correct?

Yes the package works with paket we use it at my workplace but here paket can't have packages only for some framework AFAIK and trying to trick it via groups to do that is risky (don't know if possible) as dotnet pack need to work and generate the correct dependency list per-framework.

Essentially once the library isn't distributed for netstandard1.6 anymore it can go back to paket

Also have you checked that it actually works on netcore? Only because the APIs are back doesn't mean they are working :).

Oh yes they are here :) With Core 3.0 and winforms/wpf back microsoft will have managed a 360 of removing & adding back the whole framework

image

@matthid
Copy link
Member

matthid commented Sep 7, 2018

I almost see that as volunteering for maintaining the Fake.DotNet.MSBuild package ;)

Also, I'm actually not too concerned about dropping netstandard1.6.
There was never a stable fake 5 release running on netcore1.X.
Its very unlikely that someone is using the package and the netstandard1.6 binary within. We dropped it for another package not too long ago and nobody complained ;)

We can go for it and revert to your current solution if someone complains.

@vbfox
Copy link
Contributor Author

vbfox commented Sep 7, 2018

I almost see that as volunteering for maintaining the Fake.DotNet.MSBuild package ;)

🤷🏻‍ I use it more than I change it but why not.

For the context I did that PR because I saw the other one and I had the code for that in a gist so it decided to clean it up, start using it at work instead of the MS interop package (And it's problems) and do a PR with it.

Also, I'm actually not too concerned about dropping netstandard1.6.

I wondered who was using that XD

@matthid
Copy link
Member

matthid commented Sep 7, 2018

Oh sorry I did an paket update as well for another reason. At least we will get rid of the old project files eventually. For now those files, pull and try again :)

@matthid
Copy link
Member

matthid commented Sep 15, 2018

Any interest in resolving the conflicts? ;)

@vbfox
Copy link
Contributor Author

vbfox commented Sep 17, 2018

Il look at them (again 😁)

@vbfox
Copy link
Contributor Author

vbfox commented Sep 17, 2018

Uh??? wait there was no conflicts ???

Well rebased anyway not that it cost anything.

@matthid
Copy link
Member

matthid commented Sep 17, 2018

hm either github failed to show properly or local git was clever enough to solve the conflicts? (or i failed ;))

@@ -65,6 +65,42 @@ type MSBuildDistributedLoggerConfig =
AssemblyPath : string
Parameters : (string * string) list option }

#if !NETSTANDARD1_6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This #ifdef can now dropped, since now we target netstandard2.0 instead of 1.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@matthid
Copy link
Member

matthid commented Sep 22, 2018

I guess you have tested what happens when the COM-API is not available?

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me remove the #if

@matthid matthid merged commit 1dff9bf into fsprojects:release/next Sep 22, 2018
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.

3 participants