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

Dotnet 7 #176

Closed
wants to merge 4 commits into from
Closed

Dotnet 7 #176

wants to merge 4 commits into from

Conversation

TheAngryByrd
Copy link
Member

No description provided.

Copy link
Member Author

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

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

Self review with notes for me to fix up

@@ -8,5 +8,5 @@
"test/examples",
"packages"
],
"editor.formatOnSave": true
}
// "editor.formatOnSave": true
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably need to update fantomas to 5.x because otherwise this seems to break formatting on test files.

Comment on lines +2 to +3
source https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7/nuget/v3/index.json
source https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7-transport/nuget/v3/index.json
Copy link
Member Author

Choose a reason for hiding this comment

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

Sources for hidden Microsoft.Build dlls that preview bits of F# 7 depend on. We probably gotta wait for these previews to be available.

Comment on lines +12 to +13
nuget FSharp.Core 7.0.0-beta.22473.1
nuget FSharp.Compiler.Service 42.7.100-preview.22473.1
Copy link
Member Author

Choose a reason for hiding this comment

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

Getting the latest bits. FSAC is gonna have a big diff with this.

@@ -16,13 +20,14 @@ nuget Ionide.KeepAChangelog.Tasks copy_local:true
nuget Expecto
nuget Expecto.Diff
nuget Expecto.TestResults
nuget MedallionShell ~> 1.5.0
nuget MedallionShell
Copy link
Member Author

Choose a reason for hiding this comment

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

This has some weird transitive dependencies so I had to remove the constraint. It looks kind of outdated so we may want to consider something else in the future.

Comment on lines +26 to +30
nuget Microsoft.Build 17.4.0-preview-22469-04 copy_local: false
nuget Microsoft.Build.Framework 17.4.0-preview-22469-04 copy_local: false
nuget Microsoft.Build.Utilities.Core 17.4.0-preview-22469-04 copy_local: false
nuget Microsoft.Build.Tasks.Core 17.4.0-preview-22469-04 copy_local: false
nuget Microsoft.NET.StringTools 17.4.0-preview-22469-04
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to specify all these exactly because paket wasn't downloading them for some reason.

| None ->
match args.TryGetResult Solution with
| None -> Seq.empty
(*match args.TryGetResult Solution with
Copy link
Member Author

Choose a reason for hiding this comment

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

I should put these back

Comment on lines +93 to +94
projects |> Seq.map snd |> Seq.collect (fun x -> x.Items) |> Seq.map(fun ii -> ii.ItemType, ii.EvaluatedInclude) |> Newtonsoft.Json.JsonConvert.SerializeObject |> printfn "%s"
projects |> Seq.map snd |> Seq.collect (fun x -> x.Properties) |> Seq.map(fun ii -> ii.Name, ii.EvaluatedValue) |> Newtonsoft.Json.JsonConvert.SerializeObject |> printfn "%s"
Copy link
Member Author

Choose a reason for hiding this comment

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

Added ability to get the ProjectInstance Items and Properties for further debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe should be behind another flag?

Comment on lines +812 to +816
let buildProjs =
result.ResultsByNode.Keys
|> Seq.collect (fun (pgn: ProjectGraphNode) -> seq { yield pgn.ProjectInstance })
|> Seq.toList

Copy link
Member Author

Choose a reason for hiding this comment

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

Had to move this because this could fail before we log the error from the design time build. Could add another log for how many we parsed.

@@ -1407,7 +1413,7 @@ let tests toolsPath =
testProjectSystemOnChange toolsPath "WorkspaceLoader" WorkspaceLoader.Create
testProjectSystemOnChange toolsPath "WorkspaceLoaderViaProjectGraph" WorkspaceLoaderViaProjectGraph.Create
debugTests toolsPath "WorkspaceLoader" WorkspaceLoader.Create
debugTests toolsPath "WorkspaceLoaderViaProjectGraph" WorkspaceLoaderViaProjectGraph.Create
// debugTests toolsPath "WorkspaceLoaderViaProjectGraph" WorkspaceLoaderViaProjectGraph.Create
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to add this back in

@@ -1242,7 +1242,7 @@ let testProjectSystemOnChange toolsPath workspaceLoader workspaceFactory =
)

let debugTests toolsPath workspaceLoader (workspaceFactory: ToolsPath -> IWorkspaceLoader) =
ptestCase
ftestCase
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to skip this

@TheAngryByrd TheAngryByrd mentioned this pull request Nov 7, 2022
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.

1 participant