C#: Recognize `.proj` files in autobuilder by hvitved · Pull Request #512 · github/codeql

hvitved · 2018-11-21T13:51:15Z

When determining the target of msbuild or dotnet build, first look for .proj files, then .sln files, and finally .csproj/.vcxproj files. In all three cases, choose the project/solution file closest to the root.

@calumgrant @jf205

pavgust · 2018-11-21T14:06:50Z

This pull request introduces 1 alert and fixes 1 when merging 1afc529 into 4e2d40a - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

Comment posted by LGTM.com

calumgrant

Overall extremely nice, and I think the new logic is easier to defend and probably builds more stuff. Just some trivial comments.

calumgrant · 2018-11-21T19:32:07Z

+                        Select(f => f.ProjectOrSolution);
+                    Log(Severity.Info, $"Found multiple '{extension}' files, giving up: {string.Join(", ", candidates)}.");
+                    files = new IProjectOrSolution[0];
+                    return true;


This return value is a bit weird - false means carry on and true means stop success or failure.

The return value behaves like e.g. the return value of int.TryParse(string s, out int i); the output parameter contains valid data iff the return value is true.

But actually there is no need to have both the Boolean and the enumerable, so I will rewrite.

calumgrant · 2018-11-21T19:47:50Z

-                        ThenBy(s => s.Path.Length).
+                bool FindFiles(string extension, Func<string, ProjectOrSolution> create, out IEnumerable<IProjectOrSolution> files)
+                {
+                    var allFiles = GetExtensions(extension).


allFiles -> matchingFiles ?

calumgrant · 2018-11-21T19:50:38Z

-                        Where(s => s.ProjectCount > 0).
-                        OrderByDescending(s => s.ProjectCount).
-                        ThenBy(s => s.Path.Length).
+                bool FindFiles(string extension, Func<string, ProjectOrSolution> create, out IEnumerable<IProjectOrSolution> files)


I don't think this particularly needs to be a local function.

I think local functions are nice (as opposed to private static methods) when they are only used inside one particular callable, as in this case.

calumgrant · 2018-11-21T19:54:40Z

+        /// <summary>
+        /// Gets the directory of <paramref name="path"/>, Path.GetDirectoryName().
+        /// </summary>
+        string GetDirectoryName(string path);


This probably doesn't need to be stubbed out as it doesn't touch the filesystem.

pavgust · 2018-11-22T09:28:09Z

This pull request introduces 1 alert and fixes 1 when merging fb463ae into 1739cab - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

Comment posted by LGTM.com

pavgust · 2018-11-22T10:51:54Z

This pull request introduces 1 alert and fixes 1 when merging 8648c8e into e062851 - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

Comment posted by LGTM.com

jf205

One tiny comment, otherwise all good 👍

jf205 · 2018-11-22T12:14:48Z

        /// <summary>
-        /// Full file paths of files found in the project directory.
+        /// Full file paths of files found in the project directory, as well
+        /// their distance from the project root folder. The list is sorted


as well as their distance....? (same thing for three summaries below)

pavgust · 2018-11-22T13:35:13Z

This pull request introduces 1 alert and fixes 1 when merging 9d0cd1a into 201f64e - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

Comment posted by LGTM.com

hvitved · 2018-11-22T14:26:46Z

Rebased.

pavgust · 2018-11-22T14:39:34Z

This pull request introduces 1 alert and fixes 1 when merging 743fc85 into 201f64e - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

Comment posted by LGTM.com

When determining the target of `msbuild` or `dotnet build`, first look for `.proj` files, then `.sln` files, and finally `.csproj`/`.vcxproj` files. In all three cases, choose the project/solution file closest to the root.

hvitved · 2018-11-23T08:34:41Z

Rebased again.

pavgust · 2018-11-23T08:51:00Z

This pull request introduces 1 alert and fixes 1 when merging 1939773 into 4ad5923 - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

Comment posted by LGTM.com

adityasharad

Approved by reviewers, and internal PR has passed tests.

…ix-onto-rc-31 Apply package perf fix to rc/3.1

hvitved added C# depends on internal PR This PR should only be merged in sync with an internal Semmle PR labels Nov 21, 2018

hvitved requested review from calumgrant and jf205 November 21, 2018 13:51

hvitved requested a review from a team as a code owner November 21, 2018 13:51

calumgrant suggested changes Nov 21, 2018

View reviewed changes

calumgrant reviewed Nov 21, 2018

View reviewed changes

calumgrant previously approved these changes Nov 22, 2018

View reviewed changes

jf205 reviewed Nov 22, 2018

View reviewed changes

hvitved dismissed calumgrant’s stale review via 9d0cd1a November 22, 2018 13:17

hvitved force-pushed the csharp/autobuilder/dirs-proj branch from 9d0cd1a to 743fc85 Compare November 22, 2018 14:23

hvitved added 5 commits November 23, 2018 09:32

C#: Move autobuilder into separate folder

b95d7e5

C#: Recognize .proj files in autobuilder

836daaf

When determining the target of `msbuild` or `dotnet build`, first look for `.proj` files, then `.sln` files, and finally `.csproj`/`.vcxproj` files. In all three cases, choose the project/solution file closest to the root.

C#: Address review comments

e4f68ae

C#: Guard against cyclic inclusions in project files

c3ccdfa

C#: Address review comments

1939773

hvitved force-pushed the csharp/autobuilder/dirs-proj branch from 743fc85 to 1939773 Compare November 23, 2018 08:33

adityasharad approved these changes Nov 23, 2018

View reviewed changes

adityasharad merged commit 10dc183 into github:master Nov 23, 2018

hvitved deleted the csharp/autobuilder/dirs-proj branch November 23, 2018 13:28

cklin pushed a commit that referenced this pull request May 23, 2022

Merge pull request #512 from smowton/smowton/admin/pick-performance-f…

af9c7c0

…ix-onto-rc-31 Apply package perf fix to rc/3.1

Uh oh!

Conversation

hvitved commented Nov 21, 2018

Uh oh!

pavgust commented Nov 21, 2018

Uh oh!

calumgrant left a comment

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!

pavgust commented Nov 22, 2018

Uh oh!

pavgust commented Nov 22, 2018

Uh oh!

jf205 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavgust commented Nov 22, 2018

Uh oh!

hvitved commented Nov 22, 2018

Uh oh!

pavgust commented Nov 22, 2018

Uh oh!

hvitved commented Nov 23, 2018

Uh oh!

pavgust commented Nov 23, 2018

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants