Move Rebuild to have a deny list approach by jaredpar · Pull Request #51694 · dotnet/roslyn

jaredpar · 2021-03-05T17:10:39Z

Rather than listing all of the assemblies that we support rebuilding
this moves us to listing the ones that don't support it. That makes it
much more explicit what our "work remaining" is as well as automatically
adding in new assemblies added to the project.

jaredpar · 2021-03-08T15:19:24Z

Close / reopen to see if I can fix the issue with GitHub now showing the builds correctly

jaredpar · 2021-03-08T15:25:50Z

For some reason GitHub is not displaying the latest test run results for this PR. Tried close / reopen to fix this but to no avail. Will keep this comment updated to point to the latest test run

https://dev.azure.com/dnceng/public/_build/results?buildId=1027178&view=results

jaredpar · 2021-03-08T15:26:00Z

@dotnet/roslyn-compiler PTAL

src/Tools/BuildValidator/PEReaderExtensions.cs

src/Tools/BuildValidator/Program.cs

cston · 2021-03-08T17:28:19Z

src/Tools/BuildValidator/Program.cs

excludes.Any(x => filePath.IndexOf(x, FileNameEqualityComparer.StringComparison) >= 0) [](start = 24, length = 86)

Could this be expensive if there are many excludes and many assemblies in the search paths? Perhaps create a HashSet<string> for the excludes in this method. #Closed

How would a HashSet<string> help here? In this case it's a full file path that we have to check for a substring match. Not sure how we can cache that. The returned file paths should already be unique given the way the file system is walked here. #Closed

Ah, I missed the fact we're matching substrings here.

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

src/Tools/BuildValidator/Program.cs

Rather than listing all of the assemblies that we support rebuilding this moves us to listing the ones that don't support it. That makes it much more explicit what our "work remaining" is as well as automatically adding in new assemblies added to the project.

jaredpar · 2021-03-10T18:46:28Z

@dotnet/roslyn-compiler merged with @RikkiGibson change and responded to feedback

RikkiGibson · 2021-03-10T18:56:16Z

src/Tools/BuildValidator/LocalReferenceResolver.cs

-                        if (_cache.ContainsKey(mvid))
+                        if (peInfo.IsReadyToRun)
                        {
+                            _logger.LogWarning($@"Skipping ReadyToRun image ""{file.FullName}""");


Is this more appropriate as an info than a warning? The user doesn't really control if their nuget folder contains an r2r image.

Yeah that should've been an informational

RikkiGibson · 2021-03-10T20:08:22Z

src/Tools/BuildValidator/Util.cs

+            return null;
+        }
+
+        internal static Guid? GetMvidForFile(PEReader peReader)


Feels like this could be a local function in GetPortableExecutableInfo

I would prefer to leave it this way for now. These tools usually end up grabbing MVID as a separate operation which is why I kept it as a top level method here.

RikkiGibson · 2021-03-10T20:08:47Z

src/Tools/BuildValidator/Util.cs

+            }
+        }
+
+        internal static bool IsReadyToRunImage(PEReader peReader)


Feels like this could be a local function in GetPortableExecutableInfo

Similar feedback as to GetMvidForFile.

RikkiGibson · 2021-03-10T20:32:32Z

src/Tools/BuildValidator/Util.cs

+
+namespace BuildValidator
+{
+    internal sealed record PortableExecutableInfo(string FilePath, Guid Mvid, bool IsReadyToRun);


Consider renaming the file to 'PortableExecutableInfo.cs', moving 'Util.GetPortableExecutableInfo' to a 'PortableExecutableInfo.Create' method, then eliminating the 'Util' class.

I think that having a type per file with records is going to de-value them a bit. A record is meant to be a simple concise declaration. Having them be per file means that we pretty much lose that as we have to include the copyright, namespace, using, etc ... Basically bringing more and more boiler plate back into the feature that was meant to avoid it.

Prefer we think about an approach where we can keep the simplicity here. Possibly just having a file for all the simple records as long as they remain ... simple. Push them out to separate files once they hit some levele of complexity.

RikkiGibson · 2021-03-10T20:40:20Z

src/Tools/BuildValidator/Program.cs

-        const int ExitFailure = 1;
-
-        private static readonly Regex[] s_ignorePatterns = new Regex[]
+        internal record AssemblyInfo(string FilePath, Guid Mvid)


Consider either making this type 'private' or to moving it to its own file

ghost

Auto-approval

jaredpar · 2021-03-11T18:25:09Z

Integration test is a flaky failure.

jaredpar added the Area-Compilers label Mar 5, 2021

jaredpar force-pushed the rebuild branch from 518bff5 to bbb7b3a Compare March 5, 2021 22:22

jaredpar closed this Mar 8, 2021

jaredpar reopened this Mar 8, 2021

jaredpar marked this pull request as ready for review March 8, 2021 15:24

jaredpar requested a review from a team as a code owner March 8, 2021 15:24

cston reviewed Mar 8, 2021

View reviewed changes

src/Tools/BuildValidator/PEReaderExtensions.cs Outdated Show resolved Hide resolved

cston reviewed Mar 8, 2021

View reviewed changes

src/Tools/BuildValidator/Program.cs Outdated Show resolved Hide resolved

cston reviewed Mar 8, 2021

View reviewed changes

src/Tools/BuildValidator/Program.cs Outdated Show resolved Hide resolved

jaredpar added 7 commits March 10, 2021 10:02

Fixup

f5d7556

Typo

b42a8b1

Exclude R2R refs

799cf31

More R2R updates

af41b3d

Ready for review

f35baa4

More

8040ffb

jaredpar force-pushed the rebuild branch from 9eb6659 to 8040ffb Compare March 10, 2021 18:21

jaredpar requested a review from a team as a code owner March 10, 2021 18:21

Merge and PR feedback

28c8755

RikkiGibson self-assigned this Mar 10, 2021

RikkiGibson reviewed Mar 10, 2021

View reviewed changes

PR feedback

13f47e6

cston approved these changes Mar 11, 2021

View reviewed changes

RikkiGibson approved these changes Mar 11, 2021

View reviewed changes

jaredpar added the auto-merge label Mar 11, 2021

ghost approved these changes Mar 11, 2021

View reviewed changes

jaredpar merged commit 158b7e5 into dotnet:main Mar 11, 2021

jaredpar deleted the rebuild branch March 11, 2021 18:25

allisonchou added this to the Next milestone Mar 12, 2021

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

Conversation

jaredpar commented Mar 5, 2021

Uh oh!

jaredpar commented Mar 8, 2021

Uh oh!

jaredpar commented Mar 8, 2021

Uh oh!

jaredpar commented Mar 8, 2021

Uh oh!

Uh oh!

Uh oh!

cston Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Mar 8, 2021 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaredpar commented Mar 10, 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!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Mar 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cston Mar 8, 2021 •

edited

Loading

jaredpar Mar 8, 2021 •

edited by cston

Loading