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

Convert MIDebugEngine to use SDK csproj #1075

Merged
merged 18 commits into from
Dec 16, 2020
Merged

Conversation

WardenGnaw
Copy link
Member

@WardenGnaw WardenGnaw commented Nov 4, 2020

This PR converts all the C# projects to use the new styled sdk projects.

Removed prebuild.csproj. The IL used for each specific project is added and is generated by the target which will run before resolving References.

Status Task
✔️ Analyzers
✔️ Validate MicroBuild.Core
✔️ Fix Build Pipelines
✔️ Fix The literal or constant value <...> should be passed as the 'expected' argument in the call to 'Assert.Equal(expected, actual)' in tests
✔️ Fix MIDebugPackage.pkgdef Generation
✔️ VSIX generation for SSHDebugPS

TODO List:

Status Task
😖 Fix MIDebugPackagePackage Threading Issues
Remove <PropertyGroup Condition = $(Configuration) | $(Platform) == Desktop.Debug|AnyCPU>

@gregg-miskelly
Copy link
Member

<IntermediateGeneratedOutputPath>$(IntermediateOutputPath)Generated\</IntermediateGeneratedOutputPath>

Is this still used?


Refers to: build/all_projects.settings.targets:23 in ed816ac. [](commit_id = ed816ac, deletion_comment = False)

@WardenGnaw
Copy link
Member Author

@gregg-miskelly Is it normal to require dotnet restore before building the solution? The building experience is a bit weird in VS since you have to do the restore first before building the solution.

There also seems to be an issue with MakePIAPortable and WindowsDebugLauncher not restoring when you run dotnet restore src\MIDebugEngine.sln

@WardenGnaw
Copy link
Member Author

Doesn't need to be done on this checkin, but should we delete all this crap?

Are you referencing the

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
    <DebugSymbols>true</DebugSymbols>
    <DebugType>full</DebugType>
    <Optimize>false</Optimize>
    <DefineConstants>$(DefineConstants);DEBUG;TRACE</DefineConstants>
    <ErrorReport>prompt</ErrorReport>
    <WarningLevel>4</WarningLevel>
    <PlatformTarget>AnyCPU</PlatformTarget>
  </PropertyGroup>

@gregg-miskelly
Copy link
Member

Doesn't need to be done on this checkin, but should we delete all this crap?

Are you referencing the

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
    <DebugSymbols>true</DebugSymbols>
    <DebugType>full</DebugType>
    <Optimize>false</Optimize>
    <DefineConstants>$(DefineConstants);DEBUG;TRACE</DefineConstants>
    <ErrorReport>prompt</ErrorReport>
    <WarningLevel>4</WarningLevel>
    <PlatformTarget>AnyCPU</PlatformTarget>
  </PropertyGroup>

Yes

@WardenGnaw
Copy link
Member Author

@gregg-miskelly Is it normal to require dotnet restore before building the solution? The building experience is a bit weird in VS since you have to do the restore first before building the solution.

There also seems to be an issue with MakePIAPortable and WindowsDebugLauncher not restoring when you run dotnet restore src\MIDebugEngine.sln

Looks like I need to do something like <Target Name="RestoreSDKProjects"

@gregg-miskelly
Copy link
Member

gregg-miskelly commented Nov 4, 2020

@gregg-miskelly Is it normal to require dotnet restore before building the solution? The building experience is a bit weird in VS since you have to do the restore first before building the solution.

The way it should work is that if you open the solution it should start restoring in the background. If you aren't seeing that, do you have "Tools->Options->NuGet Package Manager->Allow NuGet to download missing packages" and "Automatically check for missing packages during build in Visual Studio" checked?

It might be expected that if you build on the command line, and didn't do a dotnet restore first, it will fail. I thought they changed that, but maybe only for certain project types.

There also seems to be an issue with MakePIAPortable and WindowsDebugLauncher not restoring when you run dotnet restore src\MIDebugEngine.sln

This is probably because they aren't configured to build by default. Can you right click on the solution or project and restore them?

@Trass3r
Copy link
Contributor

Trass3r commented Nov 6, 2020

This paves the way for a .NET Core build of MIEngine, am I right?
Maybe even for Alpine support by getting rid of Mono?

@WardenGnaw
Copy link
Member Author

This paves the way for a .NET Core build of MIEngine, am I right?
Maybe even for Alpine support by getting rid of Mono?

Yep. The idea is to first move to SDK style projects but still build with .NET Framework Leaving the actual binaries unchanged. Then there will be work needed to convert the Desktop.Release (XPLAT) portion to build and publish using .NET Core.
By doing so we can remove the Mono dependency and can publish OpenDebugAD7 on any Runtime .NET Core supports.

See https://docs.microsoft.com/en-us/dotnet/core/rid-catalog#using-rids

@WardenGnaw WardenGnaw marked this pull request as ready for review December 1, 2020 00:47
@WardenGnaw WardenGnaw marked this pull request as draft December 1, 2020 00:52
This PR converts all the C# projects to use the new styled sdk projects.

https://docs.microsoft.com/en-us/dotnet/core/project-sdk/overview
Also csproj will generate pkgdef
@WardenGnaw
Copy link
Member Author

@gregg-miskelly When you have the time, can you review? The bigger changes are in 21aa199
which include creating a VSIX and auto create the pkgdef.

@gregg-miskelly
Copy link
Member

<IntermediateGeneratedOutputPath>$(IntermediateOutputPath)Generated\</IntermediateGeneratedOutputPath>

(This is still active)


In reply to: 721445297 [](ancestors = 721445297)


Refers to: build/all_projects.settings.targets:23 in 21aa199. [](commit_id = 21aa199, deletion_comment = False)

@WardenGnaw
Copy link
Member Author

@gregg-miskelly Is it normal to require dotnet restore before building the solution? The building experience is a bit weird in VS since you have to do the restore first before building the solution.
There also seems to be an issue with MakePIAPortable and WindowsDebugLauncher not restoring when you run dotnet restore src\MIDebugEngine.sln

Andrew, I missed your comment originally. Is this still an issue? Normally if you open a .NET SDK-style solution, the solution will automatically restore in the background after you open it. I think doing a build from the IDE will wait for it to finish.

Resolved. I think VS does something weird and caches the csproj style in the .vs folder. So if you go from old style to sdk style without removing the .vs folder, it skips the restore step.

Thanks for following up :)

@gregg-miskelly
Copy link
Member

Otherwise LGTM

1. Cleaned up bin output
   - Removed xunit binaries
   - Removed reference to System.Text.Json and System.Memory
   - Set 'Private: False' to referenced packages
2. Cleaned up Reference tags to System that were not needed
3. Removed unused methods that used dynamic also removed CSharp
   Reference
4. Cleaned up labels and organized some XML
Copy link
Member

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

Aside from that one nit, looks good to me.

@WardenGnaw WardenGnaw marked this pull request as ready for review December 14, 2020 19:45
@WardenGnaw
Copy link
Member Author

WardenGnaw commented Dec 14, 2020

Validating the Releases:

Status Task
✔️ VS Release - SSH / Docker / DebugAdapterHost.Launch
VS Code Release
Update Nightly Build

@WardenGnaw WardenGnaw merged commit 1462085 into main Dec 16, 2020
@WardenGnaw WardenGnaw deleted the dev/waan/ConvertToSDKProject branch February 17, 2021 19:06
@WardenGnaw WardenGnaw mentioned this pull request May 11, 2021
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