-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Cross-platform build-time code generation #3445
Conversation
6ce9342
to
610862b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ReubenBond yes, the previous VS problem was because of the usage of full .Net MSBuild by VS.
Now I tested this PR rebased on master and it worked from build.cmd and VS too!
LGTM
@ReubenBond Putting the changes from #3426 still make codegen fail with -1 error code. If Orleans.Core.Abstractions is an indirect reference like in this case only OrleansRuntime and Orleans.Transactions are reference assemblies, then we're failing. I think we need to check the flattened full list of references not just the direct references when checking Orleans reference in a codegen candidate assembly. |
50b2a07
to
985be22
Compare
} | ||
|
||
var config = new ClusterConfiguration(); | ||
var loggerFactory = new LoggerFactory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using() so that it get's disposed after done codegening
|
||
var config = new ClusterConfiguration(); | ||
var loggerFactory = new LoggerFactory(); | ||
loggerFactory.AddProvider(new FileLoggerProvider("ClientGenerator.log")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, this is putting a ClientGenerator.log alonside the code. It should be in the intermediate folder, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I ported this over from ClientGenerator in master, but I didn't like it (because of that file being generated) so I'm using ConsoleLoggerProvider now, which has required pulling in a new package.
src/ClientGenerator/CodeGenerator.cs
Outdated
|
||
var config = new ClusterConfiguration(); | ||
var loggerFactory = new LoggerFactory(); | ||
loggerFactory.AddProvider(new FileLoggerProvider("ClientGenerator.log")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as above
src/ClientGenerator/CodeGenerator.cs
Outdated
#if NET461 | ||
var generatedCode = AppDomainCodeGenerator.GenerateCode(this.options); | ||
#else | ||
var generatedCode = this.GenerateCodeInternal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a little odd how the conditional compilation was made, but it's correct. Ideally I would have the branching (but similar) code all in 1 file (or 2, but of the same shape). No big deal for this PR though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right - it's definitely a bit odd. I'll address it, thanks
@@ -0,0 +1,58 @@ | |||
<Project TreatAsLocalProperty="CodeGenDirectory;IsCore;TaskAssembly;IsCore;OutputFileName"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what it does, but IsCore is duplicated here
c6d9e9a
to
5ae4e80
Compare
@dotnet-bot test functional |
@dotnet-bot test this please |
There seems to be an issue with CI informing GitHub of build completion:
|
@dotnet-bot test functional |
🚂choo choo |
Directory.Build.props
Outdated
@@ -26,6 +26,7 @@ | |||
<GenerateDocumentationFile Condition="'$(Configuration)'=='Release'">true</GenerateDocumentationFile> | |||
<NoWarn>$(NoWarn);1591</NoWarn> | |||
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects> | |||
<GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really needed at this very general level?
Please don't merge. Our load tests are failing with this change:
|
226ab50
to
dfbfe8d
Compare
@dotnet-bot test functional |
1 similar comment
@dotnet-bot test functional |
2551c35
to
230dfe4
Compare
Could try load be a breaking change? For example if in full .net we were generating a serializer for some system type we were using, but with the new approach we end up trying to load a ref assembly and hence skipping it |
When we fail to load (because it's a ref assembly), it will return null and use the default assembly resolution logic. It's not perfect because I don't know how we should handle conflicts between assemblies the CodeGen needs and assemblies the target needs - we will try the targets first and then fallback to default behavior. |
Do you have any ideas for how to make this work, by the way? It's driving me insane. |
No, I don't. ReflectionOnly was the safe approach, I don't know now. In the future we might want to use the Roslyn approach instead of loading compiled binaries, but not sure what's best for today's approach. |
Were we ever performing reflection-only loading in the code generator? I think that aspect hasn't changed |
ah, I don't know, I just assumed we were, and maybe it was why it wasn't failing there, but TBH I don't know... In that case then we might not be breaking existing users... |
007a6f0
to
d351216
Compare
d351216
to
8090474
Compare
f09b382
to
34ae59a
Compare
Not to do in this PR, as it looks like it's ready to merge, but since these |
@jdom is it as simple as defining some overriding properties and importing the original project? It's worth experimenting with |
History: #3430 reverted #3424, this mostly reverts that revert.
The previous PR for this included a commit which moved the bootstrap codegen from net461 to netcoreapp2.0 and apparently that was a bad move (not entirely sure why, but I imagine it's because VS is using full framework MSBuild - haven't checked).
I added one extra commit: so that VS doesn't ever show the files from the bootstrap projects, we exclude all of those files during design time build. The major effect of this was duplicates when navigating around project files (eg, using ReSharper) - they're gone now.