Remove some outdated configurations in tools #52800

huoyaoyuan · 2021-04-21T14:39:42Z

Making generators run on AnyCPU
Introduced in 2016. I can't imagine any reason to run on x64 only today.
Remove AutoGenerateBindingRedirects
Defaults to on now.
Remove PlatformTargets

The only remaining non-AnyCPU project is BuildActionTelemetryTable, which depends on x86 implementation of GetHashCode. With VS2022 moving to x64, it can be also moved to AnyCPU in dev17.

huoyaoyuan · 2021-07-17T13:09:16Z

Any comment on this PR? Are these changes wanted?

src/Features/Lsif/Generator/Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.csproj

src/Test/Perf/StackDepthTest/StackDepthTest.csproj

src/Test/Perf/tests/Roslyn.PerformanceTests.csproj

src/Tools/AnalyzerRunner/AnalyzerRunner.csproj

src/Tools/IdeBenchmarks/IdeBenchmarks.csproj

src/Tools/IdeCoreBenchmarks/IdeCoreBenchmarks.csproj

sharwell · 2022-02-04T22:26:36Z

...s/Source/CompilerGeneratorTools/Source/BoundTreeGenerator/CompilersBoundTreeGenerator.csproj

@@ -2,17 +2,10 @@
 <!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE file in the project root for more information. -->
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
-    <Platform Condition="'$(Platform)' == ''">x64</Platform>
-    <PlatformTarget>x64</PlatformTarget>
-    <Platforms>x64</Platforms>


🤷‍♂️ This change doesn't seem necessary but will leave for @dotnet/roslyn-compiler to review

sharwell

Still needs two sign-offs from compiler

CyrusNajmabadi · 2022-06-03T18:19:55Z

@huoyaoyuan could you bring this PR up to date? Moving to draft in the meantime. Thanks!

huoyaoyuan · 2022-06-04T04:13:49Z

@CyrusNajmabadi Done

jcouv · 2023-02-03T00:12:10Z

Apologies. The team is small at the moment and we've not reviewed this PR in a timely fashion. As a result it went out-of-date with other changes that went in...
I'll mark this PR as draft for now and commit to review it myself if it can be brought up-to-date. Please ping when the PR is published again and we should take a look (weekly is fine). Thanks

huoyaoyuan · 2023-02-06T08:56:30Z

@jcouv Updated. Most of the conflicts are about updating net6.0 to net7.0.

Apologies. The team is small at the moment and we've not reviewed this PR in a timely fashion.

I will be interested if you consider to hire more members in the future 😄

jcouv · 2023-02-07T00:09:38Z

src/Tools/Source/CompilerGeneratorTools/CompilerTools.sln

+		{909B656F-6095-4AC2-A5AB-C3F032315C45}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{909B656F-6095-4AC2-A5AB-C3F032315C45}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{909B656F-6095-4AC2-A5AB-C3F032315C45}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{909B656F-6095-4AC2-A5AB-C3F032315C45}.Release|Any CPU.Build.0 = Release|Any CPU


nit: I noticed that some x64 configurations are left in this file. Should they be switched too?

jcouv

LGTM Thanks (iteration 9)

jcouv · 2023-02-07T00:11:18Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2023-02-07T19:12:36Z

@dotnet/roslyn-compiler for another review. Thanks

jcouv · 2023-02-08T16:48:47Z

@dotnet/roslyn-compiler for another review. Thanks

jcouv · 2023-02-09T17:53:52Z

@dotnet/roslyn-compiler for another review. Thanks

jcouv · 2023-02-14T05:22:36Z

/azp run

azure-pipelines · 2023-02-14T05:23:04Z

Azure Pipelines successfully started running 4 pipeline(s).

jcouv · 2023-02-14T20:32:47Z

Thanks for the contribution @huoyaoyuan !

jaredpar · 2023-03-09T00:08:37Z

Lesson learned from this change:

The removal of <PlatformTarget>AnyCPU</PlatformTarget> from our csproj files was not innocuous. When running a net472 application under Debug, if there is no explicit PlatformTarget then x86 will be used even when an x64 machine. This means our Rebuild leg went from running as a 64 bit app to 32 bit app. That app is right on the edge of the 2Gig boundary in terms of allocations at times and the small change in PR 67191 through it over the edge.

Putting out a PR to fix that but something we should keep in mind for future changes like this.

@jcouv, @jjonescz

This fixes the following issues in Rebuild validation 1. Adds back `<PlatformTarget>AnyCPU</PlatformTarget>` so the exe runs under x64 in CI. Running in x86 in CI is putting us right up against the memory boundary and sometimes results in crashes. 2. Changes `LocalReferenceResolver` such that directories are only enumerated twice (once for exe and one for dll). In the past every new name request caused the entire directory set to be walked again. This change saved ~8 seconds on our CI runs. 3. When there is a misc error record it in the build artifacts instead of silently failing. Related: dotnet#52800

huoyaoyuan · 2023-03-09T01:33:49Z

if there is no explicit PlatformTarget then x86 will be used even when an x64 machine.

Is Prefer32Bit ever enabled?

* Rebuild fixes This fixes the following issues in Rebuild validation 1. Adds back `<PlatformTarget>AnyCPU</PlatformTarget>` so the exe runs under x64 in CI. Running in x86 in CI is putting us right up against the memory boundary and sometimes results in crashes. 2. Changes `LocalReferenceResolver` such that directories are only enumerated twice (once for exe and one for dll). In the past every new name request caused the entire directory set to be walked again. This change saved ~8 seconds on our CI runs. 3. When there is a misc error record it in the build artifacts instead of silently failing. Related: #52800 * PR feedback

huoyaoyuan added 6 commits April 21, 2021 17:00

Make CompilersBoundTreeGenerator AnyCPU.

116d5b6

Make CSharpErrorFactsGenerator AnyCPU.

6295833

Make VisualBasicErrorFactsGenerator AnyCPU.

271e074

Make VisualBasicSyntaxGenerator AnyCPU.

585996d

Remove AutoGenerateBindingRedirects for not .NET Framework projects.

394c720

Remove unnecessary AutoGenerateBindingRedirects and PlatformTarget.

e60531a

huoyaoyuan requested review from a team as code owners April 21, 2021 14:39

dotnet-issue-labeler bot added the Area-Infrastructure label Apr 21, 2021

jinujoseph assigned sharwell Apr 21, 2021

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 21, 2021

jinujoseph added this to InQueue in IDE: CommunityPR via automation Apr 21, 2021

jinujoseph moved this from InQueue to BuddyAssigned in IDE: CommunityPR Apr 21, 2021

Merge branch 'main'

3124e91

sharwell requested changes Feb 4, 2022

View reviewed changes

sharwell moved this from BuddyAssigned to BackToCommunity in IDE: CommunityPR Feb 4, 2022

sharwell approved these changes Feb 7, 2022

View reviewed changes

IDE: CommunityPR automation moved this from BackToCommunity to LGTM Feb 7, 2022

jinujoseph moved this from LGTM to BuddyAssigned in IDE: CommunityPR Feb 7, 2022

sharwell moved this from BuddyAssigned to LGTM in IDE: CommunityPR Feb 8, 2022

sharwell added the Area-Compilers label Feb 8, 2022

jcouv assigned jaredpar Mar 8, 2022

CyrusNajmabadi marked this pull request as draft June 3, 2022 18:20

Merge branch 'main'

e2b48a5

huoyaoyuan marked this pull request as ready for review June 4, 2022 04:14

jcouv marked this pull request as draft February 3, 2023 00:12

Merge branch 'main'

aa8c842

huoyaoyuan marked this pull request as ready for review February 6, 2023 08:56

jcouv reviewed Feb 7, 2023

View reviewed changes

jcouv approved these changes Feb 7, 2023

View reviewed changes

jcouv requested a review from jaredpar February 9, 2023 19:14

jaredpar approved these changes Feb 14, 2023

View reviewed changes

jcouv enabled auto-merge (squash) February 14, 2023 05:52

jcouv merged commit f2474c1 into dotnet:main Feb 14, 2023

IDE: CommunityPR automation moved this from LGTM to May-2022 Feb 14, 2023

ghost added this to the Next milestone Feb 14, 2023

huoyaoyuan deleted the remove-legacy-config branch February 16, 2023 17:05

allisonchou mentioned this pull request Feb 17, 2023

[Automated] PRs inserted in VS build main-33417.173 #66943

Closed

RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023

jaredpar mentioned this pull request Mar 9, 2023

Rebuild fixes #67240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some outdated configurations in tools #52800

Remove some outdated configurations in tools #52800

huoyaoyuan commented Apr 21, 2021

huoyaoyuan commented Jul 17, 2021

sharwell Feb 4, 2022

sharwell left a comment

CyrusNajmabadi commented Jun 3, 2022

huoyaoyuan commented Jun 4, 2022

jcouv commented Feb 3, 2023

huoyaoyuan commented Feb 6, 2023

jcouv Feb 7, 2023

jcouv left a comment

jcouv commented Feb 7, 2023

jcouv commented Feb 7, 2023

jcouv commented Feb 8, 2023

jcouv commented Feb 9, 2023

jcouv commented Feb 14, 2023

azure-pipelines bot commented Feb 14, 2023

jcouv commented Feb 14, 2023

jaredpar commented Mar 9, 2023

huoyaoyuan commented Mar 9, 2023

Remove some outdated configurations in tools #52800

Remove some outdated configurations in tools #52800

Conversation

huoyaoyuan commented Apr 21, 2021

huoyaoyuan commented Jul 17, 2021

sharwell Feb 4, 2022

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jun 3, 2022

huoyaoyuan commented Jun 4, 2022

jcouv commented Feb 3, 2023

huoyaoyuan commented Feb 6, 2023

jcouv Feb 7, 2023

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Feb 7, 2023

jcouv commented Feb 7, 2023

jcouv commented Feb 8, 2023

jcouv commented Feb 9, 2023

jcouv commented Feb 14, 2023

azure-pipelines bot commented Feb 14, 2023

jcouv commented Feb 14, 2023

jaredpar commented Mar 9, 2023

huoyaoyuan commented Mar 9, 2023