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

A couple of features #9

Merged
merged 6 commits into from Sep 11, 2019

Conversation

@slang25
Copy link
Contributor

commented Sep 5, 2019

  • Make things a little more async
  • Search up directories for global.json to be consistent with dotnet cli
  • Check already installed SDKs and skip where appropriate
@slang25 slang25 force-pushed the slang25:global-resolve branch 2 times, most recently from ba1f294 to 2115b0d Sep 5, 2019
@JosephWoodward

This comment has been minimized.

Copy link
Owner

commented Sep 5, 2019

Oh nice one, been meaning to add checks if the SDK is already installed. I'll pull this down locally and test it on Windows before the next release.

@@ -8,6 +8,7 @@
using System.Text.Json;
using System.Threading.Tasks;
using DotNet.InstallSdk.Acquirables;
using static SimpleExec.Command;

This comment has been minimized.

Copy link
@JosephWoodward

JosephWoodward Sep 5, 2019

Owner

Nice!

@slang25

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Thanks, I've only tested on macOS as usual


var existingSdks =
dotnetInfo
.SkipWhile(x => !x.Contains(".NET Core SDKs installed:")) // TODO localize?

This comment has been minimized.

Copy link
@slang25

slang25 Sep 5, 2019

Author Contributor

It's not localized, will remove comment

Skip already installed sdks
@slang25 slang25 force-pushed the slang25:global-resolve branch from 2115b0d to 7d0774f Sep 5, 2019
@slang25

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Added silent installs (not tested at all! Yeehaw 🤠)
Need to fix failing test (_global.json is found in parent folder).

@slang25 slang25 force-pushed the slang25:global-resolve branch from c0548e8 to 4526047 Sep 6, 2019
@JosephWoodward

This comment has been minimized.

Copy link
Owner

commented Sep 6, 2019

Added silent installs (not tested at all! Yeehaw )

It'll work fine 😁 Ship it!!

@slang25

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Ha. So you have to sudo on macOS, which makes sense. Haven't tried it on Windows, other than that I think it's ready, let me know what you think.


if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
Run(installerPath, $"/install /quiet /norestart");

This comment has been minimized.

Copy link
@JosephWoodward

JosephWoodward Sep 10, 2019

Owner

What are your thoughts around making this a flag, either a flag to use headless installs, or a flag to disable a headless install? For instance:

$ dotnet install-sdk --headless true

This comment has been minimized.

Copy link
@JosephWoodward

JosephWoodward Sep 10, 2019

Owner

I'd imagine the majority of the time people would prefer for the install to be headless.

This comment has been minimized.

Copy link
@slang25

slang25 Sep 10, 2019

Author Contributor

That's a good call, and I agree that the default should probably be silent, even though this changes the current behaviour.

This comment has been minimized.

Copy link
@JosephWoodward

JosephWoodward Sep 10, 2019

Owner

Yeah, we can release it as a new version number. Better to get these kind of changes ironed out early on.

@JosephWoodward JosephWoodward merged commit a514617 into JosephWoodward:master Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.