-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for git bash running on windows. #8491
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some hints about the changes
fi | ||
|
||
say_err "OS name could not be detected: $ID.$VERSION_ID" |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -188,7 +194,7 @@ get_legacy_os_name() { | |||
fi | |||
fi | |||
|
|||
say_verbose "Distribution specific OS name and version could not be detected: $ID.$VERSION_ID" |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -581,13 +603,34 @@ copy_files_or_dirs_from_list() { | |||
extract_dotnet_package() { | |||
eval $invocation | |||
|
|||
local zip_path="$1" | |||
local out_path="$2" | |||
local download_link="$1" |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -177,6 +180,9 @@ get_legacy_os_name() { | |||
elif [ -n "$runtime_id" ]; then | |||
echo $(get_legacy_os_name_from_platform "${runtime_id%-*}" || echo "${runtime_id%-*}") | |||
return 0 | |||
elif [[ $uname == MINGW* ]]; then |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@matthid, could you provide some context as why this is needed? That is, what is the reasoning for using MINGW or CYGWIN over using Powershell or the Windows Subsystem for Linux? |
@tannergooding Sure.
We have made a really good experience with helping people to get started from zero with a single script. Now obviously for that to be best experience it should work in any shell you are most familiar with, which Imho definitely includes the git-bash. I hope that helps. |
@matthid, unfortunately, I'm not sure that answered the question. You have certainly explained why providing a good first start experience is nice (which I completely agree with). Today, dotnet-cli (and most of the dotnet repositories, in fact) do support getting started with a single script. On most of the repositories this is via
However, I'm not sure you have explained why that involves supporting the MINGW or CYGWIN bash environments specifically. There are hundreds of different shells out there, many with wildly different environments, configurations, quirks, etc. Today, you should be able to (on supported Unix platforms) just run the shell script in a POSIX compliant shell (such as |
@tannergooding Yes, I understand the concerns. But I'm very much on the pragmatic side here, so let me explain from another angle:
To come back to your examples (speaking as a potential newcomer): I would in-fact be highly frustrated right from the start if your Does that clarify why I think it makes sense to add this here? At least the above was my thinking when sending the PR. Can you explain why we wouldn't want to support the scenario considering the points above? Is it better to roll our own script instead? Maybe from one more other angle: (offtopic - history) (offtopic) Just by the way:
Wikipedia says:
So their goal is in fact to be POSIX compliant. However, I don't think this is in any way relevant to the PR. All which matters is if we in-fact want to support a maximum number of environments with these scripts or not. |
even on windows I would never see Powershell as default. |
(NOTE: This, and the above, isn't saying that this change will or will not be taken, just attempting to better understand why this change is desired, and why it should be a supported environment).
@matthid, a lot of times it isn't just as simple as "make some limited changes" because it appears to work. The .NET Core SDK isn't the only part of the .NET Core ecosystem, it is just one of the distribution mechanisms. Because it is generally undesirable to provide a "sub-par" experience across the entire ecosystem and because merging such a PR may open up the question of similar changes elsewhere, it may also need investigation as to whether or not:
These are just a few of the questions that probably need to be answered before a definitive decision is made. This is also why the general recommendation is to open up an issue to discuss the "feature request" before submitting a PR. (These guidelines are actually not very easy to find today, and I will raise an issue to try and address that).
That will depend on the outcome of these discussions 😄. If we merge the support, then you certainly won't need to.
We should definitely get this resolved to provide a better experience. It looks like several of our bash scripts require some tweaks to provide a better error experience.
Personally (my thoughts and opinions are my own and do not express the views or opinions of my employer), I don't think supporting a "maximum" number of environments is desirable. I would instead say supporting the most common set of environments is. MinGW and Cygwin are fairly popular environments on Windows and so they may be worth considering. It would also be good to clarify if this is aiming to support MinGW (which is no longer in active development), the MinGW-w64 fork, or both. |
I'm not even sure what that means. Should the linux-installer run on windows/cygwin as well? I don't think it should or needs to. I think the script should install the windows version even when running in MinGW/Cygwin. It is merely convenience.
Again I'm not sure what that means. Isn't the obtain script the only script which needs to be adopted as it only makes sure
Again I have no idea :). I cannot see any quirk here. I guess you talk about that previously the Please let me know if I can do anything to get this closer to being merged. Like I said we can make it clear that this is community maintained if that helps. Also I'm open to other suggestions on how to improve. You may have already done that "between the lines" but I'm not very good in reading that (So I'm trying to say that I have no idea what to do ;) ) Offtopic:
Note this is a general recommendation in Open-Source and I agree with that, but sometimes (especially for small PRs) it makes sense to just show the changes to better understand what we are talking about. I guess even if you make this policy more visible I'm not sure if it should apply to very small PRs like this one. TLDR: Just let me know if I can do anything :) |
I'll try to simplify my earlier comment.... You are updating one shell script to support MinGW (with a comment from another user suggesting it also be updated to support Cygwin). However, this is not the only shell script in the greater dotnet ecosystem and this could easily open the "floodgates" (so to speak) on a broader discussion on whether or not MinGW/Cygwin (or another similar tool) should be a supported environment for the rest of the shell scripts as well. This includes things like whether or not cloning the repository and running |
My stand/opinion on this is that tools should not be blocked explicitly on linux/oss world. Stuff can fail on not-supported platforms with reasonable error messages (which is always a good thing), but it should not be blocked explicitly by platform detection (like the Obviously the script (and .NET Core in general) currently takes a different approach. But Imho there is a difference between explicitly adding support for something or removing some arbitrary limitation. (Looking at the change one could argue either way, because of the Therefore the question you ask about potential support for other platforms is not valid (in my view). So I cannot answer that from there. Changing my view to your side I can see the problem and I'm not sure what to say because I cannot tell you to support MinGW/Cygwin everywhere. Obviously I'd like if you would :). Note: I'm not sure if that helps you anything but another option would be to detect windows and download/launch the PowerShell script behind the scenes.
Can't we accept this change and leave it like this? :) (I know my endless naivety) |
@tannergooding, first, this script is different than other scripts you are refering to. This is "obtain dotnet for your platform" script. Others are "build this repo on this platform for this (HOST==TARGET) or other target". So "sitting on any shell, you can obtain dotnet core for your platform" sounds like a good thing to have -> low maintenance, moderate value. BTW, is uname whitelisting intentional? If so why? Sounds like dotnet team doesn't/shouldn't care if someone is trying to use unsupported shell or platform, but shouldn't explicitly block them as well with the things like uname. Ideally the script can accept an argument to override RID, targetOS or whatever uname is doing to give leeway, instead of these unintentional and unnecessary restrictions. Makes everyone's lives easier. Unless uname is intentional for some kind of ... flood-prevention mechanism. I just searched how many pull requests have been merged in github.com/dotnet to support new OS building with uname script in past as a first step to new platform bringup (gentoo, alpine, solaris, netbsd, freebsd) in multiple repos. If this first step (i.e. bogus restriction) is removed, than many platforms which don't require any extra work would just be supported OOTB, hence win-win for everyone. "We don't officially support XYZ platform, but welcome community contributions and here to help" and likes has been famous quotes on pages under GitHub.com/dotnet and blogs.microsoft.com/dotnet. |
IIRC, there is a work item to remove most of this. The
I agree we shouldn't be explicitly blocking them. But there is also a difference from removing an artificial limitation (such as uname whitelisting) and adding explicit support for a new environment (such as
I think this is completely reasonable. It should be possible for the bash script to download the Windows, Linux, or Mac (or future supported platform) install of the SDK. The same should be true for the cmd/ps1 file as well. This ensures that users can use the script to obtain whatever bits they desire, but does not put any burden on maintaining/supporting "special code" for non-standard/non-vanilla environments (meaning something that doesn't ship with the OS, as enabled, by default). |
The conclusion of the discussion above seems to suggest that we should take a different approach with explicit options to override RID, etc, and also not block on unknown uname. I will close this based on that. |
This makes it possible to run
curl -sSL https://dot.net/v1/dotnet-install.sh | bash /dev/stdin --channel Current
in a git bash on windows (as given in the documentation)