Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Yeet FSharp.Compiler.Private #10876

Merged
merged 2 commits into from
Jan 16, 2021
Merged

Yeet FSharp.Compiler.Private #10876

merged 2 commits into from
Jan 16, 2021

Conversation

KevinRansom
Copy link
Member

This PR replaces FSharp.Compiler.Private with FSharp.Compiler.Service.

@cartermp
Copy link
Contributor

Not gonna call it "yeet FSharp.Compiler.Private"?

@KevinRansom KevinRansom changed the title Remove FSharp.Compiler.Private Yeet FSharp.Compiler.Private Jan 15, 2021
@deviousasti
Copy link
Contributor

Not gonna call it "yeet FSharp.Compiler.Private"?

I came here looking for this comment.

vzarytovskii
vzarytovskii approved these changes Jan 15, 2021
View reviewed changes
Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

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

Nice!

cartermp
cartermp reviewed Jan 15, 2021
View reviewed changes
src/fsharp/FSharp.Compiler.Service/FSharp.Compiler.Service.fsproj
<TargetFrameworks>net472;netstandard2.0</TargetFrameworks>
<OutputType>Library</OutputType>
<NoWarn>$(NoWarn);44;45;54;55;61;62;69;65;75;1204;2003;NU5125</NoWarn>
<AssemblyName>FSharp.Compiler.Service</AssemblyName>
Copy link
Contributor

Choose a reason for hiding this comment

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

the default is the project name so this is redundant

Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why net472 was added here?

src/fsharp/FSharp.Compiler.Service/FSharp.Compiler.Service.fsproj
<Tailcalls>true</Tailcalls>
<!-- .tail annotations always emitted for this binary, even in debug mode -->
<NGenBinary>true</NGenBinary>
<OtherFlags>$(OtherFlags) /warnon:3218 /warnon:1182 /warnon:3390 --maxerrors:20 --extraoptimizationloops:1 --times</OtherFlags>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is the same as it's been using before, but wouldn't it be possible to use WarnOn and stuff as properties? Feels a bit more normal is all

Copy link
Member

Choose a reason for hiding this comment

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

I've looked at this before and I don't believe there's mapping through of WarnOn in the msbuild tasks for F#. I'd love to have such a thing, personally.

Copy link
Contributor

Choose a reason for hiding this comment

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

damn, we should really do that

Copy link
Member Author

@KevinRansom KevinRansom Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@cartermp we don't have a property for it. If we did, we would have to wait until it had shipped and made it to our lkg build. I will add an issue, and add the property in a separate PR ... but we won't be able to use it here for quite a while ...

Copy link
Member

Choose a reason for hiding this comment

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

I made an issue just now, Kevin: #10882

src/fsharp/FSharp.Compiler.Service/FSharp.Compiler.Service.fsproj
@@ -918,6 +970,11 @@
<Compile Include="..\fsi\fsi.fs">
<Link>InteractiveSession/fsi.fs</Link>
</Compile>

<!-- an old API for testing the compiler and gathering diagnostics in-memory -->
<Compile Include="..\LegacyHostedCompilerForTesting.fs" Condition="'$(MonoPackaging)' != 'true'">
Copy link
Contributor

Choose a reason for hiding this comment

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

any idea if this is still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

fsharpqa test suite uses it, and is 10 times faster with it.

@baronfel baronfel mentioned this pull request Jan 15, 2021
Closed
@vzarytovskii
Copy link
Member

@cartermp shall this be yoten? Or yeeted?

@cartermp
Copy link
Contributor

Yaught

@KevinRansom KevinRansom merged commit 4b69cab into dotnet:main Jan 16, 2021
@KevinRansom
Copy link
Member Author

It has been yaught ... yoten ... yeeted ...

@dsyme
Copy link
Contributor

dsyme commented Jan 19, 2021

So it seems Microsoft.DotNet.DependencyManager has disappeared as a thing in this, and the DependencyManager protocol is now defined via reflection?

That's ok - very simplifying in terms of packaging and building these components - but it would be really nice to have a separate written RFC tooling spec for that exact reflection-vased protocol, so we're less exposed to unexpected changes, and can evolve the protocol intentionally rather than by accident.

Also it would be good to rename Microsoft.DotNet.DependencyManager to FSharp.Compiler.DependencyManagerProtocol or something. We shouldn't have Microsoft.* anything in FSharp.Compiler.Service.

dsyme
dsyme reviewed Jan 19, 2021
View reviewed changes
Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

question about LegacyMSBuildReferenceResolver

src/fsharp/FSharp.Compiler.Service/FSharp.Compiler.Service.fsproj
<Compile Include="..\LegacyMSBuildReferenceResolver.fsi">
<Link>ReferenceResolution/LegacyMSBuildReferenceResolver.fsi</Link>
</Compile>
<Compile Include="..\LegacyMSBuildReferenceResolver.fs" Condition="'$(MonoPackaging)' != 'true'">
Copy link
Contributor

Choose a reason for hiding this comment

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

So is LegacyMSBuildReferenceResolver in FSharp.Compiler.Service API surface area or not? There should be nothing conditional on MonoPackaging anymore.

I think it should not be in this DLL (because AFAIK is the thing that induces extra dependencies on MSBuild) and should instead in a separate FSharp.Compiler.Private.LegacyMSBuildReferenceResolver.dll?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dsyme, those msbuild dependencies have been in FCS for quite a while already.

@KevinRansom
Copy link
Member Author

@dsyme, there is no realkchange to the PackageManagement, beyond a namespace change from 'Microsoft.Dotnet. to 'FSharp.Compiler'. FCS had those types built in always, it';s just that now that we are using FCS internally, we don't need the Microsoft.Dotnet.DependencyManager dll at all.

@KevinRansom
Copy link
Member Author

@dsyme, I renamed the Microsoft.Dotnet. namespace to FSharp.Compiler unless I did it wrong.

@KevinRansom
Copy link
Member Author

The VS build of FSharp still uses the desktop bits.

@kerams
Copy link
Contributor

kerams commented Apr 4, 2021 •
edited
Loading

fsc.exe? Is there a problem with referencing the ,NET Standard dll for the net472 target?

@KevinRansom
Copy link
Member Author

Yes.

@KevinRansom
Copy link
Member Author

KevinRansom commented Apr 4, 2021 •
edited
Loading

  1. The desktop build supports building full windows pdbs. Whilst portable pdbs are a great replacement and should be used. There is some tooling that exist in the windows sdk that require legacy windows pdbs. So losing that would be a takeback.
  2. There is a bug in the System.Buffers nuget package, it has different assembly version numbers in the same package for the netstandard version and the desktop version. Which requires an additional binding redirect, which isn't generated for us but should be, I don't want to try to figure out why, because it makes me sad.
  3. We have a goal to sunset the desktop version of the compiler, and replace it with a frozen in time set of binaries that are opt-in able.

@KevinRansom KevinRansom deleted the extendfcs branch January 21, 2022 09:06
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
@KevinRansom
Yeet FSharp.Compiler.Private (dotnet#10876)
a647455 
* Remove FSharp.Compiler.Private

* Tweak ngen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsyme dsyme dsyme left review comments

@kerams kerams kerams left review comments

@baronfel baronfel baronfel left review comments

@cartermp cartermp cartermp approved these changes

@TIHan TIHan TIHan approved these changes

@vzarytovskii vzarytovskii vzarytovskii approved these changes

@brettfo brettfo Awaiting requested review from brettfo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@KevinRansom @cartermp @deviousasti @vzarytovskii @dsyme @kerams @baronfel @TIHan