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.

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

add check for system assemblies completion #10575

Merged
merged 3 commits into from
Dec 1, 2020
Merged

add check for system assemblies completion #10575

merged 3 commits into from
Dec 1, 2020

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Dec 1, 2020

The list of things we consider "system assemblies" was not complete for .NET 5.0, and most importantly didn't contain System.Net.WebClient, which causes failures in using the arguments generated by GetProjectOptionsFromScript when run as part of a .NET 5.0 application.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 1, 2020

Are WindowsBase/System.Windows/Microsoft.Win32.Primitives valid for non-windows FSI sessions?

systemAssemblies is not a list of references.. Instead this is a list of things which, if referenced, go in the shared "tcFrameworkImports" which is shared between other TcImports with identical file resolutions. It's primarily a heuristic to increase resource sharing. We may remove this list altogether one day if we share resources by other means sufficiently.

Putting something in this list has two effects:

  1. Any DLL with this name goes into the shared tcFrameworkImports, hence more sharing may happen (this can also cause less sharing, e.g. if we put everything in, as no to tcFrameworkImports would ever be set-identical, which is when sharing kicks in)

  2. It is reference is processed before non-framework references as it gets integrated into TcImports.

This list must have a "closure" property where any eager resolutions resolve to other assemblies in the list. The specific problem solved here is that System.Net.WebClient should be in this list, because FSharp.Core tries to resolve a reference into this assembly, and FSharp.Core is i the list. Hence its reference should be processed before everything else

For now the simplest thing is to align this list to be the union of GetProjectReferencesFromScript for the different target frameworks we care about.

@KevinRansom
Copy link
Member

@dsyme
I'm assuming that you got the list of assemblies to add to the sdkAssemblies from the list in the packs directory which, is fine. Having the sdkAssemblies table contain each framework list is somewhat duplicative. It for sure means a longer scan through the source for us to to figure out if a particular assembly is in the list. I imagine that formatting it in alphabetical sorted order with each assembly only once would be more practical.

@dsyme the thing that I am most curious about is the FSharp.Core dependency on System.Web.Client. As far as I can tell neither fsi, fsharp.compiler.private nor fsharp.core depend on it. The reason it is in the set is that for use sdk refs, we specify the entire set of netsdk assemblies as references.

I would prefer, that rather than having this list, grow endlessly as the sdk grows, that we just add code to compute it similarly to what we do for usesdk and shrink the list back down again.

The test is good.

I will merge this, and consider a permanent solution.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 1, 2020

I'm assuming that you got the list of assemblies to add to the sdkAssemblies from the list in the packs directory which, is fine. Having the sdkAssemblies table contain each framework list is somewhat duplicative. It for sure means a longer scan through the source for us to to figure out if a particular assembly is in the list. I imagine that formatting it in alphabetical sorted order with each assembly only once would be more practical.

TBH I got the lists from the respective results of GetProjectOptionsFromScript.

@dsyme the thing that I am most curious about is the FSharp.Core dependency on System.Web.Client. As far as I can tell neither fsi, fsharp.compiler.private nor fsharp.core depend on it. The reason it is in the set is that for use sdk refs, we specify the entire set of netsdk assemblies as references.

I think the references go via netstandard.dll

In .NET 5.0, netstandard.dll (`"c:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\5.0.0\ref\net5.0\netstandard.dll") has this:

.assembly extern System.Net.WebClient
{
  .publickeytoken = (CC 7B 13 FF CD 2D DD 51 )                         // .{...-.Q
  .ver 5:0:0:0
}

I would prefer, that rather than having this list, grow endlessly as the sdk grows, that we just add code to compute it similarly to what we do for usesdk and shrink the list back down again.

Yup

I will merge this, and consider a permanent solution.

cool

@dsyme dsyme merged commit 256bb26 into dotnet:main Dec 1, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Dec 1, 2020

@baronfel If you could publish a new FCS I'd be really grateful, when you have a chance

@KevinRansom
Copy link
Member

@dsyme publishing fcs is on us now.

@KevinRansom
Copy link
Member

The code needs to flow to dev16 branch, then I will push a new fcs.

@auduchinok
Copy link
Member

@KevinRansom What's the connection between dev16 branch and publishing new FCS version from main?

@KevinRansom
Copy link
Member

@auduchinok, we originally were going to publish everything from the active release branch. However ... that would require the dogfood build, which I realized once I had thought about it a bit. We haven't really got the hang of releasing fcs yet, so we haven't identified the correct process. TBH my preference would be releasing from a servicing branch with only cherry picked features coming through. Because as pointed out by @baronfel elsewhere API deletions have already occured which requires a major version number change. Given that those type of changes happen in master continuously anyway .. more thought is required for the actual get it out the door step.

In the meantime, we will ship this one from master and try to figure out what to do in the future.

@auduchinok
Copy link
Member

auduchinok commented Dec 2, 2020

@KevinRansom I think it'll be really great if changes regarding publishing the community bits are announced clearly. It's been published from master/main previously, and if it's going to be changed to, say, using some stable/outdated branch, it really should be discussed somewhere, since many community projects tend to rely on having recent changes in FCS.

@KevinRansom
Copy link
Member

@auduchinok well, ideally we would preview from release/dev16.9 and service from release/16.8. So that we could protect most developers from churn and not have a major version number that rotated faster than a rocket. On the other hand, it is work.

@baronfel
Copy link
Member

baronfel commented Dec 2, 2020

The community (speaking for FSAC, fantomas, fsharplint, and FAKE here since those are the ones I generally maintain) is generally ok with rapidly-increasing version numbers, so long as we get something for it ;) It's just a number, and I think we much more value rapid delivery over something cosmetic like that.

@KevinRansom
Copy link
Member

KevinRansom commented Dec 2, 2020

@baronfel --- I think a reasonable approach to take is to publish previews out of main branch to nuget.
I.e. 39.0.0-betasomecrazynumber
And have a servicing branch for fcsserving to publish servicing fixes to 38.0.n with fixes cherry picked into fcs servicing that don't break apis.

@brettfo , @cartermp what do you think, will this work?

@KevinRansom
Copy link
Member

We can always modify this proposal if it doesn't work out to be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants