Skip to content
This repository has been archived by the owner on Dec 12, 2020. It is now read-only.

feat: Allow override of the generated files output path #228

Closed
wants to merge 2 commits into from

Conversation

robkroll
Copy link

@robkroll robkroll commented Apr 28, 2020

Allow override of the output directory where CGR.Tool writes generated files. Defaults to existing value $(IntermediateOutputPath) so no change for existing users.

Partially fixes #95

Copy link
Collaborator

@amis92 amis92 left a comment

Choose a reason for hiding this comment

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

This looks good. I'd like to ask for a few more things to finish this off:

  1. An example in samples with an appropriate check somewhere (either a target in csproj, or a build script with the check) that the files were actually saved to the other, specified directory.
  2. Document the feature in the README, close to the current MSBuild extensibility documentation.
  3. Put an entry in CHANGELOG.md about this feature as an addition.

I think this should be enough to polish and merge this PR.

@@ -64,6 +64,8 @@
<CodeGenerationRoslynToolPath
Condition="'$(GenerateCodeFromAttributesToolPathOverride)' != ''">$(GenerateCodeFromAttributesToolPathOverride)</CodeGenerationRoslynToolPath>
<CodeGenerationRoslynToolPath>$(CodeGenerationRoslynToolPath.Trim())</CodeGenerationRoslynToolPath>
<CodeGenerationRoslynFilesOutputPath
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd prefer CodeGenerationRoslynToolOutputPath.

@amis92
Copy link
Collaborator

amis92 commented Apr 29, 2020

I'd also like to suggest that maybe a warning should be raised (with a code to help suppression) when:

  1. There are multiple TargetFrameworks specified and TargetFramework value is not part of the specified Path.
  2. Configuration value is not part of the Path.

These two cases will result in files being overwritten and an undefined result in the end.

@amis92 amis92 requested a review from AArnott April 29, 2020 16:30
@robkroll
Copy link
Author

Thanks for the feedback, I'll work on those changes. One question in the meantime: why does CGR generate an AssemblyAttributes file (.e.g .NETStandard,Version=v2.0.AssemblyAttributes.APNPx2rD.generated.cs).

The file content is only the auto-generated banner and then:

using System;
using System.Reflection;

But it seems that this file is picked up for generation again by CGR. So if the CodeGenerationRoslynToolOutputPath folder is set to $(MsBuildProjectFolder) it will regenerate this file over and over (appending .generated.generated...) until it reaches the 260 char limit and breaks.

@amis92
Copy link
Collaborator

amis92 commented Apr 30, 2020

So if the CodeGenerationRoslynToolOutputPath folder is set to $(MsBuildProjectFolder) it will regenerate this file over and over (appending .generated.generated...) until it reaches the 260 char limit and breaks.

This will be the case anyway if you'll not exclude those files from default Compile include manually. This should also probably be highlighted in documentation.

CGR runs before CoreCompile (at which time the AssemblyAttributes.cs is already generated), and it takes in all Compile items (ItemGroup->Compile).

So, by default, if you set the CodeGenerationRoslynToolOutputPath to $(MsBuildProjectFolder), only running build once is a supported scenario (and only if you're not multitargeting).

Because every subsequent build will include the generated files in the Compile item by default, and we'll generate duplicates of those files, as you've noticed.

Our compilation setup wasn't ever meant to have the files saved to the project directory. It's designed from the ground up as a per-Compilation runner.

@robkroll
Copy link
Author

robkroll commented Apr 30, 2020

CGR runs before CoreCompile (at which time the AssemblyAttributes.cs is already generated), and it takes in all Compile items (ItemGroup->Compile).

Ok, so all files are passed to CGR.Tool before the compile. But if a generator outputs, say, an empty class definition (and the generated class is therefore not attributed for code generation) it will get included in a subsequent call to CGR.Tool, but as it's not marked for generation it's not passed to a generator and nothing happens with it -- no new files are produced.

Only if a generated file is attributed for code generation do we get this recursive .generated.generated.generated... problem. For example this happens with the DuplicateWithSuffixGenerator from the example. This for sure would be worth documenting.

Except for the AssemblyAttributes.cs file, this seems to be generated regardless.

@amis92
Copy link
Collaborator

amis92 commented Apr 30, 2020

as it's not marked for generation it's not passed to a generator and nothing happens with it -- no new files are produced

Speculating here (no proof), but I'm pretty sure we create the new files no matter if there were any generators run or trigger attributes found. It may be the case that they get re-generated and overwritten. It all just adds to this being a totally not supported scenario. :(

@amis92
Copy link
Collaborator

amis92 commented May 8, 2020

I think we could mitigate at least partially the infinite-regeneration scenario by changing slightly how we consume the Compile list:

  1. Map input paths to generated paths
  2. Exclude input paths that exist in the generated paths collection.
  3. Generate g.cs only for items left on Compile (filtered).

@robkroll would you be interested in adding this in this PR?

@AArnott do you think this'd work?

@AArnott
Copy link
Owner

AArnott commented May 8, 2020

Yes, I think that's promising @amis92. Another option, IIRC, is that we only generate for "all" source files right now because of an ItemDefinitionGroup that turns it on by default for all Compile items. A project that's doing something special like @robkroll proposes likely wants to turn this default off and explicitly select with metadata which Compile items should actually result in code generation.

@robkroll robkroll closed this Jun 15, 2020
@robkroll robkroll deleted the feat/output-path-override branch June 15, 2020 12:34
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.

Generate files in project instead of obj directory
3 participants