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

Update logic of resolving SdkReferenceAssemblies. #2639

Merged
merged 1 commit into from
Jan 30, 2022

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jan 26, 2022

dotnetRoot.Directory.FullName
</> "packs"
</> "Microsoft.NETCore.App.Ref"
</> this.ResolveSdkRuntimeVersion()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not something I want to tackle in this PR but this.ResolveSdkRuntimeVersion() is just going to throw if no global.json is present. This feels wrong, could the version not be extracted from running dotnet --version instead if no global.json is present?

Copy link
Contributor

Choose a reason for hiding this comment

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

agree here, we can't assume the presence of global.json. the safest path is to invoke the dotnet found from the new code you added and call --version. Proj-Info also does this here

Copy link
Contributor

@mclark1129 mclark1129 Mar 25, 2022

Choose a reason for hiding this comment

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

Why doesn't dotnet --version work for all cases? Why care if global.json exists or not?

@nojaf
Copy link
Contributor Author

nojaf commented Jan 26, 2022

Oh, and I tested this on my local project and it worked on my machine 😊.

@nojaf
Copy link
Contributor Author

nojaf commented Jan 26, 2022

Failing tests feel unrelated,

Failed:  3
	Fake.DotNet.Cli.IntegrationTests.Template tests/can install and run the template/can build a buildtask-dsl inline-dependencies template
	Fake.DotNet.Cli.IntegrationTests.Template tests/can install and run the template/can build a buildtask-dsl file-dependencies template
	Fake.DotNet.Cli.IntegrationTests.Template tests/can install and run the template/fails to build a target that doesn't exist

@yazeedobaid
Copy link
Collaborator

@nojaf Thanks for the PR, the template integration tests started to fail not sure why!
This happens after we upgrade to net 6.0.101 in FAKE. I'll try to look at them later today.

Thanks

@yazeedobaid
Copy link
Collaborator

Tests should be fixed in this PR.

@nojaf
Copy link
Contributor Author

nojaf commented Jan 26, 2022

Thanks @yazeedobaid, I'll rebase when #2640 got merged in.
Assuming my PR goes green then, could we have a hotfix release for fake-cli once this is merged?

@yazeedobaid
Copy link
Collaborator

@nojaf Yeah sure, if all goes well then we can release this as a hotfix.

@yazeedobaid
Copy link
Collaborator

Should we also use this algorithm in Fake.DotNet.Cli module? Specifically in the findPossibleDotnetCliPaths method

let findPossibleDotnetCliPaths dotnetCliDir = seq {
let fileName = if Environment.isUnix then "dotnet" else "dotnet.exe"
yield!
ProcessUtils.findFilesOnPath "dotnet"
|> Seq.filter File.Exists
|> Seq.filter (fun dotPath -> dotPath.EndsWith fileName)
let userInstalldir = defaultUserInstallDir </> fileName
if File.exists userInstalldir then yield userInstalldir
let systemInstalldir = defaultSystemInstallDir </> fileName
if File.exists systemInstalldir then yield systemInstalldir
match dotnetCliDir with
| Some userSetPath ->
let defaultCliPath = userSetPath @@ fileName
match File.Exists defaultCliPath with
| true -> yield defaultCliPath
| _ -> ()
| None -> () }

cc: @matthid , @baronfel

@baronfel
Copy link
Contributor

@yazeedobaid yes, but with one modification - the current algorithm supports checking the user-local install directories if as a last-resort, but the code from proj-info doesn't. I think that behavior should be kept for probing purposes, and TBH it should be added to proj-info as well. e.g. adding an |> Option.orElseWith tryFindFromDefaultUserDirs to this lazy code

@mclark1129
Copy link
Contributor

Guessing this is related, although not entirely the same as the original issue mentioned on the PR: #2641

@yazeedobaid
Copy link
Collaborator

@nojaf I have merged #2640 can you please update our branch from release/next

Thanks

@nojaf
Copy link
Contributor Author

nojaf commented Jan 30, 2022

@yazeedobaid rebased.

@yazeedobaid
Copy link
Collaborator

Thanks @nojaf
For using this algorithm in Fake.DotNet.Cli module as well, I have opened an issue for it #2644 we can track it in that issue.
I'll merge this now and release it

Thanks again

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.

Reference assemblies are located in the wrong folder on Windows
4 participants