Extract Rebuild library by RikkiGibson · Pull Request #51709 · dotnet/roslyn

RikkiGibson · 2021-03-06T01:39:09Z

No description provided.

RikkiGibson · 2021-03-06T02:25:19Z

I added the new project to the solution via dotnet sln add and BuildBoss gives me the following failure:

Processing Solution D:\workspace\_work\1\s\Compilers.sln ... FAILED
Processed 71 projects
Project Rebuild.csproj should have GUID 9a19103f-16f7-4668-be54-9a1e7a4f7556 but has fae04ec0-301f-11d3-bf4b-00c04f79efbc

Processing Solution D:\workspace\_work\1\s\Roslyn.sln ... FAILED
Processed 197 projects
Project Rebuild.csproj should have GUID 9a19103f-16f7-4668-be54-9a1e7a4f7556 but has fae04ec0-301f-11d3-bf4b-00c04f79efbc

I don't know what this means, but I guess I might need to try again to add the project from Visual Studio. @jaredpar

jaredpar · 2021-03-08T13:54:58Z

I don't know what this means, but I guess I might need to try again to add the project from Visual Studio.

It just means that the GUID used in the SLN file is incorrect. The line which declares the project , and likely the subsetquent consupmions, use the wrong GUID.

jaredpar · 2021-03-08T13:56:19Z

src/Compilers/Core/Rebuild/Rebuild.csproj

+    <OutputType>Library</OutputType>
+    <TargetFrameworks>netcoreapp3.1;netstandard2.0</TargetFrameworks>
+    <PlatformTarget>AnyCPU</PlatformTarget>
+    <Nullable>enable</Nullable>


Is this needed? Think it should just come from the Directory.Build.props file

BuildValidator also does this. I'll remove both entries in a subsequent PR.

cston · 2021-03-08T18:52:41Z

src/Compilers/Core/Rebuild/BuildConstructor.cs

-    /// An abstraction for building from a MetadataReaderProvider
-    /// </summary>
-    internal class BuildConstructor
+    public class BuildConstructor


public [](start = 4, length = 6)

Should the types in this assembly be kept as internal with InternalsVisibleTo() access added for BuildValidator?

We do intend for external users to be able to consume this library. I want to punt on the decision of which members to reduce to internal accessibility until we get closer to the point where the public API is ready for review.

We do intend for external users to be able to consume this library.

If these types will be public, we should also review the namespace names when the public API is reviewed.

In reply to: 589681272 [](ancestors = 589681272)

cston · 2021-03-08T19:36:06Z

src/Compilers/Core/Rebuild/BuildConstructor.cs

-    /// An abstraction for building from a MetadataReaderProvider
-    /// </summary>
-    internal class BuildConstructor
+    public class BuildConstructor


class [](start = 11, length = 5)

sealed

Will address in follow-up.

cston · 2021-03-08T19:36:15Z

src/Compilers/Core/Rebuild/CompilationOptionsReader.cs

-    }
-
-    internal class CompilationOptionsReader
+    public class CompilationOptionsReader


class [](start = 11, length = 5)

sealed

Will address in follow-up.

cston · 2021-03-08T20:05:19Z

src/Compilers/Core/Rebuild/BuildConstructor.cs

+                        var methodSymbols = typeSymbol
+                            .GetMembers(mainMethodName)
+                            .OfType<IMethodSymbol>();
+                        return methodSymbols.FirstOrDefault();


FirstOrDefault [](start = 45, length = 14)

Do we need to handle the case where there are multiple "Main" methods in the type? Could we simply use producedCompilation.GetEntryPoint() instead (and avoid looking at optionsReader as well)?

I'm interested in looking more closely at this, as I believe it is buggy, however in this PR I will just migrate this as-is.

cston · 2021-03-08T20:30:00Z

src/Tools/BuildValidator/Program.cs

+            var parseResult = JsonConvert.DeserializeAnonymousType(Encoding.UTF8.GetString(sourceLinkUTF8), new { documents = (Dictionary<string, string>?)null });
+            var sourceLinks = parseResult.documents.Select(makeSourceLink).ToImmutableArray();
+
+            if (sourceLinks.IsDefault)


sourceLinks.IsDefault [](start = 16, length = 21)

Does ToImmutableArray() ever return default?

I do not think so.

RikkiGibson added 4 commits March 5, 2021 16:23

Add Rebuild.csproj

9891cb4

Extract local source/reference resolvers from build constructor

2177839

Move BuildConstructor

c108f50

Move emit from CompilationDiff to BuildConstructor

4c70889

ghost added the Area-Compilers label Mar 6, 2021

RikkiGibson added 3 commits March 5, 2021 17:40

revert rebuild script

23b8918

Remove unused references, adjust target frameworks

b5e175c

Remove Newtonsoft from Rebuild lib

e74a79f

RikkiGibson marked this pull request as ready for review March 6, 2021 01:58

RikkiGibson requested review from a team as code owners March 6, 2021 01:58

jaredpar approved these changes Mar 8, 2021

View reviewed changes

fix sln files

6a9631b

cston reviewed Mar 8, 2021

View reviewed changes

cston approved these changes Mar 8, 2021

View reviewed changes

RikkiGibson merged commit f7f683a into dotnet:main Mar 8, 2021

RikkiGibson deleted the rebuild-lib branch March 8, 2021 20:47

This was referenced Mar 11, 2021

Rebuild does not obtain the entry point correctly #51802

Closed

Make Rebuild types sealed where possible #51803

Open

allisonchou added this to the Next milestone Mar 12, 2021

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

Conversation

RikkiGibson commented Mar 6, 2021

Uh oh!

RikkiGibson commented Mar 6, 2021

Uh oh!

jaredpar commented Mar 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants