Skip to content

Conversation

@fadimounir
Copy link
Contributor

@fadimounir fadimounir commented Dec 5, 2019

Broken by #445 (cc @ViktorHofer)

Using 3.0 for now enables us to build from VS, which is an important scenario for our team currently

@dotnet/crossgen-contrib

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 5, 2019

@fadimounir sorry for the break here. Can't you just install the 5.0 SDK instead? (locally)

@fadimounir
Copy link
Contributor Author

@ViktorHofer I haven't tried installing 5.0. Does that work with VS today?

@ViktorHofer
Copy link
Member

It should yes, but let me try it first.

@fadimounir
Copy link
Contributor Author

It should yes, but let me try it first.

I just tried it. It doesn't work

<OutputType>Library</OutputType>
<AssemblyName>ILCompiler.ReadyToRun</AssemblyName>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
<TargetFramework Condition="'$(BuildingInsideVisualStudio)' == 'true'">netcoreapp3.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

OK, I think the right fix would be to downgrade this project entirely. So to set TargetFramework to netcoreapp3.0 not just for inside VS as it was before. We can upgrade at a late rpoint. Same for crossgen2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think it was a good idea when you upgraded it to 5.0, since this is the shipping configuration.

The netcoreapp3.0 is just a temporary internal scenario used by our team, and we can remove it when dotnet5 starts working with VS.

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

I'm fine with the change but I would prefer to hear from Viktor that he's fine with whatever we're doing before merging in.

@ViktorHofer
Copy link
Member

I will take a look early tomorrow. We can merge this in and if something points out I will let you know.

@fadimounir fadimounir merged commit f10276e into dotnet:master Dec 6, 2019
@fadimounir fadimounir deleted the EnableBuildingOfCrossgen2InVS branch December 19, 2019 20:42
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants