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

Need a common way to reference CMakeLists.txt projects from a csproj #4008

Closed
hughbe opened this issue Sep 24, 2019 · 21 comments · Fixed by #4533
Closed

Need a common way to reference CMakeLists.txt projects from a csproj #4008

hughbe opened this issue Sep 24, 2019 · 21 comments · Fixed by #4533

Comments

@hughbe
Copy link
Contributor

hughbe commented Sep 24, 2019

This is of use in dotnet/winforms#1932

See dotnet/winforms#1932 (comment)

We'd basically like to extract the behavior present in coreclr (https://github.com/dotnet/coreclr/blob/master/Documentation/building/test-configuration.md) where you can write

<ProjectReference Include="../NativeDll/CMakeLists.txt" />

This is useful to many different .NET Core projects, not just coreclr. I'd like to use it in winforms but also WPF would need to be able to test interop scenarios similarily

/cc @RussKie @AaronRobinsonMSFT @jkoritzinsky

@RussKie
Copy link
Member

RussKie commented Sep 25, 2019

/cc: @AdamYoblick @zsd4yr

@zsd4yr
Copy link
Contributor

zsd4yr commented Oct 7, 2019

@hughbe What specifically is the ask from Arcade here? Are you saying that we cannot currently have a reference to a CMakeList.txt in our project references? (i.e. we cannot add a native reference?)

@zsd4yr
Copy link
Contributor

zsd4yr commented Oct 7, 2019

Or are you saying we can but that it should be a common CMakeList.txt (i.e. the one used by CoreCLR should be the same one we use here) and therefore we need a place to put that common CMakeList?

@RussKie
Copy link
Member

RussKie commented Oct 8, 2019

No, it is that we can't build c++ projects as part of the existing pipeline.
There is a discussion in dotnet/winforms#1932 that contains details

@AaronRobinsonMSFT
Copy link
Member

@RussKie and @hughbe This request is a bit vague with respect to what would be expected from the Arcade team. There are multiple levels of support implied here so we should be very clear about what is desired.

CMake is a meta build system tool. This means it generates the build scripts for actually building. Would Arcade be responsible for install CMake? Which version would be supported?

During build generation, CMake makes some assumptions about what kind of scripts to generated based on the system it is running (e.g. On Windows it looks for Visual Studio from newest to oldest - I believe 2010). On non-Windows systems gcc is usually queried for, but I think clang is also an option. macOS has its own set of nuances that CMake attempts to abstract from the developer and also by default relies on XCode being installed. This leads to a question about system configuration. Are you expecting Arcade to install the appropriate tool chains? If so, which ones? Perhaps Arcade should assume the system is canonical in configuration and what should be there is there.

This then brings up a question regarding platform/flavor configuration which would necessitate some canned CMake files be provided by Arcade to properly define the multiple flavors (i.e. RET, CHK, DBG) and ensure that the generated files align with how the managed components will reference them and discover the their outputs.

These are only two questions regarding some additional details about expectations for this request. I want to be clear that I would very much welcome Arcade taking care of all the above, I just want everyone to understand the scope of this specific request.

@RussKie
Copy link
Member

RussKie commented Oct 8, 2019

Thank you for providing the insights. It makes it easier to know what kind of questions we need to ask:)

@hughbe
Copy link
Contributor Author

hughbe commented Oct 29, 2019

Ok. I'll try to explain what we're trying to do in winforms and then I'll detail how I think extracting coreclr's logic to arcade could help with this.

  1. What we're trying to do
    We have a C++ project containing exported methods that are p-invoked by a C# test project. Running dotnet build on the C# test project, we want to automatically build the C++ project and copy it to the bin/output directory of the C# project so that we can reference it using DllImport.

  2. How can we achieve this?
    Essentially what we're asking for is a way to ProjectReference a C++ project. CMake seems to be the best way to be able to do this. I am proposing that we pull out the logic present in coreclr that lets you do <ProjectReference Include="../NativeDll/CMakeLists.txt" />

I believe there is some logic e.g. in https://github.com/dotnet/coreclr/blob/master/tests/src/Directory.Build.targets#L113-L197

  <Target Name="CopyNativeProjectBinaries" Condition="'$(_CopyNativeProjectBinaries)' == 'true'">

     <ItemGroup>
        <NativeProjectBinaries Condition="'$(RunningOnUnix)' != 'true'" Include="$(NativeProjectOutputFolder)\**\*.*" />

        <!-- ############################################################### -->
        <!-- The following is unix only. It is required because the unix test-->
        <!-- build, unlike the windows test build, do not place the binaries -->
        <!-- built into a separate Debug/Checked/Release directory. Therefore-->
        <!-- we must filter the folder to only include dynamic libraries,    -->
        <!-- static libraries and executables.                               -->
        <!--                                                                 -->
        <!-- Please take care when modifying the following lines of code.    -->
        <!-- At a minimum to test any changes to the below, please run a     -->
        <!-- pri1 test build.                                                -->
        <!-- ############################################################### -->

        <!-- Include everything and then filter. -->
        <NativeProjectBinariesMatched Condition="'$(RunningOnUnix)' == 'true'" Include="$(NativeProjectOutputFolder)\**\*.*" />

        <!-- Filter executables on unix -->
        <NativeProjectBinariesExeFilter Condition="'$(RunningOnUnix)' == 'true' and $([System.Text.RegularExpressions.Regex]::IsMatch(['%(Identity)', `(.*\/)([^.]+)$`))" Include="@(NativeProjectBinariesMatched)" />
        <!-- Filter out the *Make* files -->
        <NativeProjectBinariesExeFilterRemovedMakeFile Condition="'$(RunningOnUnix)' == 'true' and !$([System.Text.RegularExpressions.Regex]::IsMatch(['%(Identity)', `.*Makefile.*`))" Include="@(NativeProjectBinariesExeFilter)" />

        <!-- Filter out the CMakeFiles files -->
        <NativeProjectBinariesExeFilterRemovedCMakeFile Condition="'$(RunningOnUnix)' == 'true' and !$([System.Text.RegularExpressions.Regex]::IsMatch(['%(Identity)', `.*CMakeFiles.*`))" Include="@(NativeProjectBinariesExeFilterRemovedMakeFile)" />

        <!-- Filter .dylib files on OSX -->
        <NativeProjectBinariesDyLibFilter Condition="'$(__BuildOS)' == 'OSX' and $([System.Text.RegularExpressions.Regex]::IsMatch(['%(Identity)', `(.*\/).*\.dylib`))" Include="@(NativeProjectBinariesMatched)" />

        <!-- Filter .so files on Linux -->
        <NativeProjectBinariesDyLibFilter Condition="'$(__BuildOS)' == 'Linux' and $([System.Text.RegularExpressions.Regex]::IsMatch(['%(Identity)', `(.*\/).*\.so`))" Include="@(NativeProjectBinariesMatched)" />

        <!-- Filter static lib files on Unix -->
        <NativeProjectBinariesStaticLibFilter Condition="'$(RunningOnUnix)' == 'true' and $([System.Text.RegularExpressions.Regex]::IsMatch(['%(Identity)', `(.*\/).*\.a`))" Include="@(NativeProjectBinariesMatched)" />

        <!-- Merge the filtered lists -->
        <NativeProjectBinaries Condition="'$(RunningOnUnix)' == 'true'" Include="@(NativeProjectBinariesExeFilterRemovedCMakeFile)" />
        <NativeProjectBinaries Condition="'$(RunningOnUnix)' == 'true'" Include="@(NativeProjectBinariesDyLibFilter)" />
        <NativeProjectBinaries Condition="'$(RunningOnUnix)' == 'true'" Include="@(NativeProjectBinariesStaticLibFilter)" />

     </ItemGroup>

     <Error  Text="The native project files are missing in $(NativeProjectOutputFolder) please run build from the root of the repo at least once"
             Condition="'@(NativeProjectBinaries)' == ''" />

     <Copy
        SourceFiles="@(NativeProjectBinaries)"
        DestinationFiles="@(NativeProjectBinaries -> '$(OutDir)%(Filename)%(Extension)')"
        SkipUnchangedFiles="$(SkipCopyUnchangedFiles)"
        OverwriteReadOnlyFiles="$(OverwriteReadOnlyFiles)"
        Retries="$(CopyRetryCount)"
        RetryDelayMilliseconds="$(CopyRetryDelayMilliseconds)"
        UseHardlinksIfPossible="$(CreateHardLinksForCopyFilesToOutputDirectoryIfPossible)">
        <Output TaskParameter="DestinationFiles" ItemName="FileWrites" />
     </Copy>
  </Target>

  <Target Name="ResolveCmakeNativeProjectReference"
          Condition="'@(ProjectReference)' != ''"
          BeforeTargets="ConsolidateNativeProjectReference;BeforeResolveReferences;BeforeClean" >
     <ItemGroup>
        <NativeProjectReference Include="%(ProjectReference.Identity)" Condition="$([System.String]::Copy(%(ProjectReference.FileName)).ToUpper()) == 'CMAKELISTS'" />
        <ProjectReference Remove="%(NativeProjectReference.Identity)" />
        <NativeProjectReferenceNormalized Include="@(NativeProjectReference -> '%(FullPath)')" />
     </ItemGroup>
  </Target>

  <Target Name="ConsolidateNativeProjectReference"
          Condition="'$(_CopyNativeProjectBinaries)' == 'true'"
          BeforeTargets="Build" >
     <ItemGroup Condition="'@(NativeProjectReferenceNormalized)' != ''">
        <NativeProjectOutputFoldersToCopy Include="$([System.String]::Copy('%(NativeProjectReferenceNormalized.RelativeDir)').Replace($(SourceDir),$(__NativeTestIntermediatesDir)\src\))" Condition="'$(RunningOnUnix)' == 'true'" />
        <NativeProjectOutputFoldersToCopy Include="$([System.String]::Copy('%(NativeProjectReferenceNormalized.RelativeDir)').Replace($(SourceDir),$(__NativeTestIntermediatesDir)\src\))$(Configuration)\" Condition="'$(RunningOnUnix)' != 'true'" />
     </ItemGroup>

    <Message Text= "Full native project references are :%(NativeProjectReferenceNormalized.Identity)" />
    <Message Text= "Native binaries will be copied from :%(NativeProjectOutputFoldersToCopy.Identity)" />

    <MSBuild Projects="$(MSBuildProjectFile)" Targets="CopyNativeProjectBinaries" Properties="NativeProjectOutputFolder=%(NativeProjectOutputFoldersToCopy.Identity)" Condition="'@(NativeProjectReference)' != ''" />

  </Target>

  <Target Name="CopyAllNativeProjectReferenceBinaries" DependsOnTargets="ResolveCmakeNativeProjectReference;ConsolidateNativeProjectReference" />

To answer your specific questions

Would Arcade be responsible for install CMake? Which version would be supported?

I think this is possible in the same way that coreclr does (https://github.com/dotnet/coreclr/blob/master/eng/common/native/install-cmake.sh). The version supported should be consistent across .NET Core projects and obviously depends on the requirements of all the projects

Are you expecting Arcade to install the appropriate tool chains? If so, which ones?

I would expect Arcade to use the user's default toolchain (same behaviour as in coreclr). Its the user's responsibility for this.

@markwilkie
Copy link
Member

Thanks @hughbe ! One of the things I would be interested in, is what (if any) common functionality Arcade should provide across repos. In general, the "rule of thumb" is that if something is shared - then it's likely a good candidate for our shared (go figured) infrastructure - e.g. Arcade.

Any chance that after extracting/copying/plagiarising and otherwise stealing the good work from coreclr, we could circle back around and see if (or how) that bit should fit back into Arcade?

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Oct 29, 2019

@hughbe Okay seems like something that isn't going to force too much policy into other repos. Here are two additional thing that must also be addressed/considered.

  1. The MSBuild logic doesn't generate or build the native project, it just references the output binaries. Would you expect the new Arcade logic to trigger a build from within MSBuild or is that up to the repo owner to have run a pre-buildnative process? The build fails if the native binaries aren't present is that the desired behaviour as well?

  2. Somewhat related to the build issues in (1), the native projects must have a well defined output layout so that the MSBuild logic knows about the different build flavours/platforms and where to find those references. This means that Arcade would need to provide a customized CMake file that all native projects ingest and follow so their install outputs are in the appropriate location.

@hughbe
Copy link
Contributor Author

hughbe commented Oct 29, 2019

The MSBuild logic doesn't generate or build the native project, it just references the output binaries. Would you expect the new Arcade logic to trigger a build from within MSBuild or is that up to the repo owner to have run a pre-buildnative process? The build fails if the native binaries aren't present is that the desired behaviour as well?

Yeah I noticed this. Ideally, yes. Would simplify things greatly

Somewhat related to the build issues in (1), the native projects must have a well defined output layout so that the MSBuild logic knows about the different build flavours/platforms and where to find those references. This means that Arcade would need to provide a customized CMake file that all native projects ingest and follow so their install outputs are in the appropriate location.

Do we not already have a standard layout artifacts/bin/projectname?

@AaronRobinsonMSFT
Copy link
Member

Yeah I noticed this. Ideally, yes. Would simplify things greatly

That is a bit more complicated since there would be complexity with parallel build, but of course would be preferred. It does increase the work and testing burden significantly.

Do we not already have a standard layout artifacts/bin/projectname?

We do indeed. However, that needs to be propagated to all CMake generated projects so that those projects are also aware of the layout. In order to do that CMake needs to know about the additional build flavour (Checked) and ensure that configuration is described properly with respect to compiler flags et al. Also of note, I don't believe the default Release build configuration in CMake passes the "generate symbol files" compiler flag, so that configuration's flags must also be updated in a global way.

There is also the IDE support question. If the user loads the .csproj file in VS or some other IDE, is the expectation that the ProjectReference tags will be respected? Probably not a blocker, but this is similar to my initial post about defining the expectations for the work done by the Arcade team. @markwilkie's suggestion of creating a proof-of-concept (POC) in the WinForms repo so the Arcade team can understand a second example of desired support would be the ideal way to make the case.

@jkoritzinsky
Copy link
Member

I've been working locally on making a new SDK to add to Arcade that would drive our CMake builds in the runtime repo. I could definitely add a feature that enables referencing a CMakeLists.txt from another project through this SDK.

I'm going to do a little bit of work testing it out with CoreCLR before I put out a PR here.

@markwilkie
Copy link
Member

@jkoritzinsky - I'm going through and looking at issues w/ no activity for a bit. Is this still something you'd like to do?

@jkoritzinsky
Copy link
Member

Yes. Unless @hughbe has any additional feedback, I believe this is ready to be merged.

@jkoritzinsky
Copy link
Member

Well, #4533 is ready to be merged to fix this.

@markwilkie
Copy link
Member

Ah yes, thanks for reminding me of this PR. Has all the feedback been addressed?

Getting signoff from @mmitche , @alexperovich , and @jkotas is sufficient for me to merge.

@jkoritzinsky
Copy link
Member

I believe I've addressed all the feedback, but I'm ok waiting for explicit signoffs.

@riarenas
Copy link
Member

@jkotas are you ok with this?

@jkotas
Copy link
Member

jkotas commented Apr 23, 2020

See #4533 (comment)

@markwilkie
Copy link
Member

@jkoritzinsky - looks like there still an open bit of feedback from @jkotas

@jkoritzinsky
Copy link
Member

It looks like I've addressed all of Jan's feedback from what I can see. There's a little bit of feedback from @mmitche that I'll go address now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants