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

Build-time code generation for .NET Core #3424

Merged
merged 6 commits into from
Sep 22, 2017

Conversation

ReubenBond
Copy link
Member

@ReubenBond ReubenBond commented Sep 21, 2017

Note: this does not yet support building Orleans.sln on Linux or other platforms but it does allow building projects which use Orleans on Linux.

The build fails the first time (and subsequently succeeds) when building in VS. We can fix that by setting Project Dependencies in VS so that Orleans relies on ClientGenerator.Bootstrap, but that breaks cmdline builds. It's a known issue: dotnet/msbuild#2274

Removed BootstrapCodegen.proj because it doesn't play nicely with nuget / msbuild. Instead, I added 3 *.Bootstrap projects under a folder called BootstrapBuild. Each one corresponds to a project in src and they link their cs files from there. I added those projects to Orleans.sln so that msbuild correctly restores dependencies and initializes its internal structures.

The linked sources in those projects are hidden using properties on the <Compile> csproj item. The <Link> metadata is used to hide the directory structure. This is just to reduce visual clutter in VS: only the Dependencies node is visible in the image below.

image

To make builds work on on .NET Core in the absence of AppDomains, we spawn a new process for code generation. Therefore we need a way to find the correct dotnet executable. This is done using an MSBuild task called GetDotNetHost.

Hopefully we can eliminate bootstrapping in the near future :)

@ReubenBond
Copy link
Member Author

Ok, I've implemented a workaround for the MSBuild bug: now it should happily build in VS / cmdline without trouble.

@sergeybykov sergeybykov added this to the 2.0.0-beta milestone Sep 21, 2017
Copy link
Contributor

@attilah attilah left a comment

Choose a reason for hiding this comment

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

Generally, I am not happy with the duplicated projects, but we talked about it and since it is probably a temporary solution I'm fine with it for the greater good, that's coming.

I just added a few naming comments.

@@ -43,6 +44,9 @@
<PropertyGroup>
<!-- System packages -->
<SystemRuntimeVersion>4.3.0</SystemRuntimeVersion>

<MicrosoftBuildVersion>15.3.409</MicrosoftBuildVersion>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It should go to Microsoft* section in props, alphabetical order ;-) Just to make if easier to locate these.

@@ -0,0 +1,65 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Filename should be GetDotNetHost.cs

@@ -23,17 +23,17 @@
</ItemGroup>

<ItemGroup>
<None Include="CreateOrleansTables_MySQL.sql">
<None Include="CreateOrleansTables_MySql.sql">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why changing the casing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it fails on non-Windows builds otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, but then we should correct the casing of the files itself, not pascal casing SQL in the csproj IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because the casing here is incorrect.

@@ -0,0 +1,3 @@
<Project>
Copy link
Contributor

Choose a reason for hiding this comment

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

Microsoft.Orleans.CodeGenerator.Build.targets should be a better naming here, since we're removing the duplicate "Orleans" from everywhere, project names, package names, assembly names, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

The name aligns to the NuGet package name

Copy link
Contributor

Choose a reason for hiding this comment

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

But not by our new standards, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

if the package name changes, we can change this. Otherwise it won't be automatically added to the project when it's installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless we modify the name when we pack it, like:
<PackagePath>build\$(PackageId).targets</PackagePath>. I'm not sure that's better, though - it might be confusing.

@ReubenBond
Copy link
Member Author

Pushed a bunch of commits to address feedback.

The solution also is sort-of buildable on nix now, but that is a non-goal of this PR. To make it make it build cleanly on nix, we would need to remove the Windows-only projects (Service Fabric, etc) from the sln and switch TestProjectTargetFramework in Directory.Build.props to something nix compatible, like netcoreapp2.0.

@attilah attilah merged commit 26e0a1e into dotnet:master Sep 22, 2017
jason-bragg added a commit to jason-bragg/orleans that referenced this pull request Sep 22, 2017
benjaminpetit pushed a commit that referenced this pull request Sep 22, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants