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

Solution for .NET Standard using linked files #2177

Merged
merged 16 commits into from
Sep 20, 2016

Conversation

jdom
Copy link
Member

@jdom jdom commented Sep 20, 2016

This new solution contains the working place for the migration effort to .NET Standard and .NET Core (#2143).

The vNext tree has a separate solution (src/Orleans.vNext.sln) with several projects that have links to the code files in the main (non-vNext) solution.
Currently not all of the original projects were migrated to target .NET Standard, and we'll add them as we need and can (for example, we first need to get a few basic test projects up so that we can start asserting the port is working as expected).

We mainly use 2 different compilation constant to differentiate between the 2 solutions (via conditional compilation):

  • NETSTANDARD: When something is just different between the 2 targets. Long term we want to avoid multi-targeting library projects for full .NET Framework and .NET Standard. So it means that this compilation constant will eventually be dropped, but we do not expect this soon (at least not before the WALK phase defined in issue [WIP] Orleans 2.X #2143).
  • NETSTANDARD_TODO: short term temporary conditional compilation to get the project to build, but requires fixing before that feature is functional. This was mainly added to get the project bootstrapped and compiling so that the community can start contributing these pieces that were temporarily left out.

Things coming soon:

  • I will soon add this solution to be built in CI to avoid contributions going to the main solution to break vNext.
  • I'll start adding some basic test projects. The ones that require a silo will take some more time, as there are several things needed for this work.
  • This should now be ready for people submitting PRs against it, especially with fixes for stuff marked with the NETSTANDARD_TODO compilation constant.

See this comment for rationale of this approach.

Note to committers (and reviewers): please do not squash this into a single commit. If there's feedback to address (most likely), I'll add new commits, but when this has been approved an it's ready to be merged, I'll just rebase and clean the commit history, since I tried to track each of the remaining issues into separate commits (which is a nice way to review too, if you want to go 1 commit at a time)

Add Orleans and OrleansRuntime
Add codegen projects and execution (these do not generate anything currently)
Link to all files in Orleans
Target .NET Standard 1.5
AutoGenerateBindingRedirects
Add README-CoreClr-port explaining the port process.
@jdom jdom added this to the CoreCLR milestone Sep 20, 2016
@ReubenBond
Copy link
Member

@dotnet-bot test this please

@galvesribeiro
Copy link
Member

Thanks @jdom! will review tomorrow

@ReubenBond
Copy link
Member

Is the eventual plan to have just one .sln, or to maintain two? I'm not sure I see the intended path here

@galvesribeiro
Copy link
Member

@ReubenBond this is temporary so we can work in parallel in master. When we have become .Net Standard compatible, we just replace the projects/solution.

@ReubenBond
Copy link
Member

Hopefully this is a short-lived change. I appreciate the push to move .NET Standard work into the mainline branch - it should help us move much faster.

@jdom
Copy link
Member Author

jdom commented Sep 20, 2016

Ideally this will not be needed once we finish the walk phase, and we are confident that we can replace the binaries with the ones targeting .Net standard. So it will be medium-term lived, or as long as it takes us to reach the end of that 2nd phase

@@ -97,11 +97,14 @@ private static void ProcessAssembly(Assembly assembly)
{
Logger.Verbose3("Processing assembly {0}", assemblyName);
}

#if !NETSTANDARD
Copy link
Member

Choose a reason for hiding this comment

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

Below, this is guarded by !NETSTANDARD_TODO - why the discrepancy?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no reflection only load, so we won't have this scenario in .net standard (that's why it's not a TODO item at least here).
I'll review tomorrow if there's another reflection only load check that I incorrectly marked as TODO and fix it, thanks. The reality is that previously they were all just NETSTANDARD, and I recently started rebasing and crafting the commits so they add the NETSTANDARD_TODO one instead, so it is very likely that I missed a few where it made sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, there were several NETSTANDARD_TODO comments related to reflection only that I changed to NETSTANDARD (and rebased). They were all in commit Remove some usages of ReflectionOnly and our type resolvers in .NET Standard (which is now 00de92f8fd8da2e824b06383352c0b9261d91209)

<ItemGroup />
<Import Project="$(MSBuildExtensionsPath32)\Microsoft\Portable\$(TargetFrameworkVersion)\Microsoft.Portable.CSharp.targets" />
<PropertyGroup>
<NuGetTargetMoniker>.NETStandard,Version=v1.5</NuGetTargetMoniker>
Copy link
Member

Choose a reason for hiding this comment

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

(I'm probably mistaken, but) the project itself doesn't seem to be targeting .NET Standard 1.5 directly, it looks like a PCL
image

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, this is by design. It is an official hack explained here: https://docs.microsoft.com/en-us/dotnet/articles/core/tutorials/target-dotnetcore-with-msbuild

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be replaced once the final msbuild tooling for .net standard is in place. In the meantime it allows us to do things like compile time codegen with the existing targets, instead of migrating everything to the dotnet CLI tooling which will be completely changed very soon

Copy link
Member

Choose a reason for hiding this comment

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

That explains it :) Sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, I'm glad you downloaded it locally for reviewing, as it is hard to get a sense of how it works without it. Let me know if you have any issues building, as this csproj stuff made me do a few hacks (the other one is that project references don't work well with this, so it is using project build dependencies to define the project build order, and then dependent projects reference the built dll in the output path).

Copy link
Member

Choose a reason for hiding this comment

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

Both trees build fine & BVTs pass in main tree. Now I'm trying to get IL-serializer working on top of it :)

@ReubenBond
Copy link
Member

Assuming that we have consensus on the general approach (creating a vNext subdirectory with projects which link to files from the main tree) and that we will clean up the #ifs before 2.0 (there should be an open issue on it), then LGTM. I'm still a little wary of introducing mess, but maybe the benefit outweighs the cost.

@jdom
Copy link
Member Author

jdom commented Sep 20, 2016

I understand, and I also explored many options to avoid this. Nevertheless this ended up being the most workable approach.
A little bit of the rationale behind this approach:

  • using csproj allows us to be unblocked even before the final tooling RTMs (otherwise we would have to re-do the tooling once xproj goes back to csproj)
  • we also decided to work in the master branch instead of creating a separate branch:
    • otherwise most of our contributors would be unaware of it and unknowingly break when submitting PRs for the non-vNext solution (or even if aware, they would need to submit changes against 2 different branches, which is not at all practical).
    • this way we continue to support the good old non-vNext solution until we reach the end of the WALK phase and have a usable 2.0 release.
    • because we support both targets in the same branch, then we require this extra vNext solution, and also the linked files (there were other otions considered, such as using conditionals in the main csproj themselves, but that's probably even more messier, but I could stand corrected).

If getting there would be a week's worth of work, then this intermediate vNext solution would not be needed, but we know that's impossible. We need something that temporarily builds even if it is not functional for the community (and rest of our core team) contributions to start coming, otherwise I'm the single bottleneck to get everything 100% working before we can push to master.

@xiazen
Copy link
Contributor

xiazen commented Sep 20, 2016

Good job !@jdom . Will take a closer look tomorrow !

@galvesribeiro
Copy link
Member

galvesribeiro commented Sep 20, 2016

@ReubenBond When Julian first came with the new .sln idea I was reluctant about multiple #ifs myself but the benefits of being on master as he stated worth a lot more IMHO however, I think we can save some of those #ifs if we bump our framework version to 4.6.1. but we will eventually hit Azure users that will need use startup tasks to install it. This is the only downside of not having a diff branch - stuck on 4.5.1. Many things like AsyncLocal<T> for the RequestContext are only available on 4.6.1. Also, there are other PRs in place that would remove many of those TODOs or shims like the AssemblyLoader one.

Overall, LGTM. I'll be back soon and start working on that as well.

@jdom jdom force-pushed the netstandard-same-tree branch from 4b51fcf to a096c49 Compare September 20, 2016 17:19
@jdom jdom force-pushed the netstandard-same-tree branch from a096c49 to 0766ee1 Compare September 20, 2016 19:03
@sergeybykov sergeybykov merged commit 80e1f5e into dotnet:master Sep 20, 2016
@jdom jdom deleted the netstandard-same-tree branch September 21, 2016 16:03
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 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.

6 participants