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

Resolve symlinks while searching for dotnet #2662

Merged
merged 3 commits into from
Apr 6, 2022

Conversation

Smaug123
Copy link
Contributor

@Smaug123 Smaug123 commented Feb 27, 2022

Description

Resolve the path to dotnet fully (traversing symlinks) when trying to find the .NET reference assemblies.

This is necessary when, for example, you've installed dotnet using Nix. On my machine, for example:

➜ which dotnet
/etc/profiles/per-user/patrick/bin/dotnet
➜ readlink $(which dotnet)
/nix/store/2rhkamzmpmri3f91fcdvdgy8bbqacwyx-home-manager-path/bin/dotnet
➜ readlink $(readlink $(which dotnet))
/nix/store/q3w1pk4li282x1fml7rs8p93in5gq3ig-dotnet-sdk-6.0.100/bin/dotnet 

Only this final dotnet has a packs directory above it.

  • New (API-)documentation for new features exist (Note: API-docs are enough, additional docs are in help/markdown)

  • unit or integration test exists (or short reasoning why it doesn't make sense)

    Note: Consider using the CreateProcess API which can be tested more easily, see https://github.com/fsharp/FAKE/pull/2131/files#diff-4fb4a77e110fbbe8210205dfe022389b for an example (the changes in the DotNet.Testing.NUnit module)

  • boy scout rule: "leave the code behind in a better state than you found it" (fix warnings, obsolete members or code-style in the places you worked in)

  • (if new module) the module has been linked from the "Modules" menu, edit help/templates/template.cshtml, linking to the API-reference is fine.

  • (if new module) the module is in the correct namespace

  • (if new module) the module is added to Fake.sln (dotnet sln Fake.sln add src/app/Fake.*/Fake.*.fsproj)

  • Fake 5 API guideline is honored

@yazeedobaid
Copy link
Collaborator

@Smaug123 Thanks for the PR 🚀

I suppose there is no easy way to add tests for this?

Thanks

@Smaug123
Copy link
Contributor Author

Well, quite :P I couldn't find one, but I'm not at all familiar with the FAKE codebase.

@yazeedobaid
Copy link
Collaborator

@Smaug123 The integration tests for SdkAssemblyResolver are in FAKE/src/test/Fake.Core.IntegrationTests/Fake.DotNet.sdkAssemblyResolver.fs

Please let me know if you need more information

Thanks

@Smaug123
Copy link
Contributor Author

Smaug123 commented Mar 1, 2022

I don't suppose you'd be interested if I refactored so that this could be unit tested instead?

I'm shaving yaks here; FAKE doesn't work on my Nix+Apple Silicon setup, which means I can't dotnet fake build, so it's extremely frictionful to write tests of any kind. This PR is intended to fix one of the reasons why my setup doesn't work, but the unrelated #2626 is showstopping and I'm not willing to switch to x64 emulation or a non-Nix installation, so my development workflow here and in Fantomas tends to boil down to "stop using FAKE/Paket, add package references to all .fsproj files in the transitive footprint of the file I'm working on, and build using dotnet instead".

@yazeedobaid
Copy link
Collaborator

boy scout rule: "leave the code behind in a better state than you found it" (fix warnings, obsolete members or code-style in the places you worked in)

This point is one of the hints/requirements for sending a PR to FAKE. So, yes absolutely, if you can find room for improvement please go ahead. Every help/improvement counts.

We are currently working on the next major version of FAKE, v6. We are concerned mainly in two points:

  1. Improving modules and checking their state -> this is in progress.
  2. Improving FAKE runner. This mainly includes experimenting with how we can utilitze dotnet fsi out of the box feature(s) to resolve dependencies instead of using Paket and trying to resolve reference assemblies for a Dotnet SDK installation in the FAKE runner.

You are welcome to help in that regard also. We are using release/v6 branch for this work.

Thanks

@Smaug123
Copy link
Contributor Author

@yazeedobaid I'm not at all happy with this test - the whole thing seems to be hanging together by a thread - but there's a chance you'd consider this good enough to go in?

@yazeedobaid
Copy link
Collaborator

@Smaug123 Yes thanks a lot
and sorry for the late response

@yazeedobaid yazeedobaid merged commit 04235f0 into fsprojects:release/next Apr 6, 2022
@Smaug123 Smaug123 deleted the follow-symlinks branch September 7, 2022 20:35
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.

2 participants