Skip to content

Reducing compiler directives (NETSTANDARD) - #14510

Merged
KevinRansom merged 17 commits into
mainfrom
reducing-compiler-directive-usage
Jan 13, 2023
Merged

Reducing compiler directives (NETSTANDARD)#14510
KevinRansom merged 17 commits into
mainfrom
reducing-compiler-directive-usage

Conversation

@T-Gro

@T-GroT-Gro commented Dec 22, 2022

Copy link
Copy Markdown
Member

Eliminate usage of NETSTANDARD compiler directives for main libraries where we have target frameworks netstandard(20/21) only => NETSTANDARD was defined in both cases, so the compiler always took only 1 branch of the #if .

  • Fsharp.Compiler.Service
  • Fsharp.Core

Reduce usage of NETSTANDARD conditionals in tests where needed.

Encapsulate #if - then #else pattern for tests that have been:

  • Either NetCoreApp or DESKTOP, never both
  • A xUnit Fact before (This PR does not enforce moving from nunit to xunit for tests where this did not apply)
  • The test project already referenced Test.Utilities before (a new dependency not added)

@T-Gro
T-Gro marked this pull request as ready for review December 22, 2022 18:38
@T-Gro
T-Gro requested a review from a team as a code ownerDecember 22, 2022 18:38
Comment threadsrc/Compiler/Driver/CompilerConfig.fs Outdated
KevinRansom
KevinRansom previously approved these changes Dec 26, 2022

@KevinRansomKevinRansom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.

Comment threadsrc/Compiler/Driver/CompilerConfig.fs Outdated
0101
0101 previously approved these changes Dec 27, 2022
@T-Gro

Copy link
Copy Markdown
MemberAuthor

/run fantomas

@dotnetdotnet deleted a comment from github-actionsBotDec 27, 2022
@T-Gro

Copy link
Copy Markdown
MemberAuthor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@T-Gro

Copy link
Copy Markdown
MemberAuthor

@KevinRansom :
I have to rollback your proposal.

When changing runtimeRoot (from typeof.Assembly.Location to GetRuntimeDirectory() ), a test for FSI /usesdkrefs**-** starts failing with:

Error creating evaluation session: System.AggregateException: One or more errors occurred. (Assembly: The system type 'System.Array' was required but no referenced system DLL contained this type, full path: System.Array)

@T-Gro

Copy link
Copy Markdown
MemberAuthor

@KevinRansom : I have to rollback your proposal.

When changing runtimeRoot (from typeof.Assembly.Location to GetRuntimeDirectory() ), a test for FSI /usesdkrefs**-** starts failing with:

Error creating evaluation session: System.AggregateException: One or more errors occurred. (Assembly: The system type 'System.Array' was required but no referenced system DLL contained this type, full path: System.Array)

@KevinRansom : Funnily, there was a bug related to trimming separators which only revealed itself after doing this change.
Should be able to fix it and keep your proposal :)

0101
0101 previously approved these changes Jan 2, 2023
0101
0101 previously approved these changes Jan 5, 2023
@T-Gro
T-Gro marked this pull request as draft January 12, 2023 17:12
@T-Gro
T-Gro marked this pull request as ready for review January 12, 2023 19:40
@T-GroT-Gro added this to the January-2023 milestone Jan 12, 2023
@T-Gro
T-Gro requested a review from 0101January 12, 2023 20:46
@KevinRansom
KevinRansom merged commit 55af4e8 into mainJan 13, 2023
@T-Gro
T-Gro deleted the reducing-compiler-directive-usage branch January 25, 2023 15:11
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants

@T-Gro@0101@KevinRansom@psfinaki@vzarytovskii