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

Rename GenerateReferenceAssemblySources -> GenerateReferenceAssemblySource #6157

Merged
merged 3 commits into from
Feb 23, 2021

Conversation

KirillOsenkov
Copy link
Member

When #6141 updated from Microsoft.DotNet.BuildTools.GenAPI to Microsoft.DotNet.GenAPI our reference assembly source generation logic stopped working.

Turns out the new NuGet package targets relies on singular spelling to insert itself after PrepareForRun:

Update our spelling to singular everywhere now that we're using the new package.

…ource

When #6141 updated from Microsoft.DotNet.BuildTools.GenAPI to Microsoft.DotNet.GenAPI our reference assembly source generation logic stopped working.

Turns out the new NuGet package targets relies on singular spelling to insert itself after PrepareForRun:

<PrepareForRunDependsOn Condition="'$(GenerateReferenceAssemblySource)' == 'true'">

Update our spelling to singular everywhere now that we're using the new package.
@KirillOsenkov
Copy link
Member Author

The command-line options for the GenAPI tool must have changed. The tool is running now (good) but fails:

  Unhandled Exception: McMaster.Extensions.CommandLineUtils.UnrecognizedCommandParsingException: Unrecognized option '-
  e'
     at McMaster.Extensions.CommandLineUtils.CommandLineProcessor.HandleUnexpectedArg(String argTypeName, String argVal
  ue)
     at McMaster.Extensions.CommandLineUtils.CommandLineProcessor.ProcessOption()
     at McMaster.Extensions.CommandLineUtils.CommandLineProcessor.ProcessNext()
     at McMaster.Extensions.CommandLineUtils.CommandLineProcessor.Process()
     at McMaster.Extensions.CommandLineUtils.CommandLineApplication.Parse(String[] args)
     at McMaster.Extensions.CommandLineUtils.CommandLineApplication.Execute(String[] args)
     at Microsoft.DotNet.GenAPI.Program.Main(String[] args) in /_/src/Microsoft.DotNet.GenAPI/Program.cs:line 54
  StringTools -> C:\msbuild\artifacts\bin\StringTools\Debug\netstandard2.0\Microsoft.NET.StringTools.dll
C:\Users\kirill\.nuget\packages\microsoft.dotnet.genapi\5.0.0-beta.19467.25\build\Microsoft.DotNet.GenAPI.targets(47,5)
: error MSB3073: The command ""C:\Users\kirill\.nuget\packages\microsoft.dotnet.genapi\5.0.0-beta.19467.25\build\\..\to
ols\net472\Microsoft.DotNet.GenAPI.exe" "C:\msbuild\artifacts\bin\Microsoft.Build.Framework\Debug\net472\Microsoft.Buil
d.Framework.dll" --lib-path "C:\Users\kirill\.nuget\packages\microsoft.netframework.referenceassemblies.net472\1.0.0\bu
ild\.NETFramework\v4.7.2;C:\Users\kirill\.nuget\packages\microsoft.netframework.referenceassemblies.net472\1.0.0\build\
.NETFramework\v4.7.2\Facades" --out "C:\msbuild\ref\Microsoft.Build.Framework\net\Microsoft.Build.Framework.cs"  -exclu
deApiList:"C:\msbuild\ref\ExcludeAPIList.txt" -excludeAttributesList:"C:\msbuild\ref\ExcludeAttributeList.txt" -headerF
ile:"C:\msbuild\ref\LicenseHeader.txt"" exited with code -532462766. [C:\msbuild\src\Framework\Microsoft.Build.Framewor
k.csproj]

@benvillalobos mind taking over this PR and fixing this up? Thanks!

@KirillOsenkov KirillOsenkov added the Area: Our Own Build Problems affecting the build or build infrastructure of the MSBuild repo itself. label Feb 16, 2021
@benvillalobos
Copy link
Member

The args that need to be updated are:
--exclude-api-list
--exclude-attributes-list
--header-file

Comment on lines +1446 to +1447
private readonly object _dummy;
private readonly int _dummyPrimitive;
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why it's now adding these dummy fields?

Copy link
Member

Choose a reason for hiding this comment

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

Great question, I have no idea! @epananth any ideas why this might happen 🙂? I believe this source generator is owned by arcade.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed with @riarenas, he mentioned that this lives in arcade but we do not own it. @ericstj might know more about this.

Copy link
Member

Choose a reason for hiding this comment

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

These are added here for compile-time compat. This is added to structs that have private fields that need to be initialized.

https://github.com/dotnet/arcade/blob/ca7fab569267ed3bc73360882d652d119aae5653/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs#L159-L184

If we don't include these it is observable, and potentially dangerous, impact on project that consume them:

  • Allows pointers to be created for structs with fields of a reference type.
  • Prevents the compiler from catching cyclic struct layout problems.
  • Breaks C# definite assignment checking.
  • Allows structs using [FieldOffset] to verify when they should not.
  • Prevents developers from correctly planning for interop scenarios.

Here is the issue that made us add this: dotnet/runtime#16402

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 18, 2021
@ericstj ericstj requested a review from safern February 18, 2021 01:26
@ladipro ladipro merged commit 56626eb into master Feb 23, 2021
@KirillOsenkov KirillOsenkov deleted the dev/kirillo/GenerateReferenceAssemblySource branch February 23, 2021 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Our Own Build Problems affecting the build or build infrastructure of the MSBuild repo itself. merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants