This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Use POSIX "command" instead of non-standard "which" #8823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GNU which might not be installed on a minimal GNU/Linux installation. Refs dotnet#6994 and dotnet#7025.
|
I believe the only flaw in the prior PR for this (which broke the official build) was that it should have been getting the full path for the two variables CC and CXX. I wasn't able to verify that at the time though, and because nobody needed this until now, it fell low on my list of priorities 😄. The reason for using hash was to maintain consistency with other places where we use it for checking whether commands exist. That said, I'm okay with this change if it works. |
dagood
reviewed
Jan 6, 2017
| @@ -110,8 +110,7 @@ if [ ! -e $__INIT_TOOLS_DONE_MARKER ]; then | |||
| __DOTNET_LOCATION="https://dotnetcli.blob.core.windows.net/dotnet/Sdk/${__DOTNET_TOOLS_VERSION}/${__DOTNET_PKG}.${__DOTNET_TOOLS_VERSION}.tar.gz" | |||
| # curl has HTTPS CA trust-issues less often than wget, so lets try that first. | |||
| echo "Installing '${__DOTNET_LOCATION}' to '$__DOTNET_PATH/dotnet.tar'" >> $__init_tools_log | |||
| which curl > /dev/null 2> /dev/null | |||
| if [ $? -ne 0 ]; then | |||
| if command -v curl > /dev/null; then | |||
There was a problem hiding this comment.
This looks backwards to me, and the official builds seem to have tried to use wget when the machine should use curl.
There was a problem hiding this comment.
Good catch, Sorry:I just wanted to cleanup non-idiomatic shell-code while doing the which->curl change. I will never merge multiple changes in one commit again
😞
There was a problem hiding this comment.
No worries, I can see how easy it would be to mix up those changes. It will be nice to have the more idiomatic/straight forward approach from now on.
|
My thought on why this passed CI is that CI machines might happen to have @dleeapho FYI on an interesting case that CI/official prereq disparity can cause. |
This was referenced Jan 6, 2017
am11
added a commit
to am11/cli
that referenced
this pull request
Feb 25, 2017
`which` is an external utility. `hash` or `command -v` are built-in General Commands. See http://stackoverflow.com/a/677212/863980 Related: dotnet/coreclr#8823 & dotnet/corert#2831 I chose `hash` here as `run-build.sh` is already using it.
am11
added a commit
to am11/cli
that referenced
this pull request
Feb 25, 2017
`which` is an external utility. `hash` or `command -v` are built-in General Commands. See http://stackoverflow.com/a/677212/863980 Related: dotnet/coreclr#8823 & dotnet/corert#2831 I chose `hash` here as `run-build.sh` is already using it.
jack-pappas
added a commit
to jack-pappas/visualfsharp
that referenced
this pull request
Feb 27, 2017
Use the POSIX built-in 'command' instead of 'which'; the latter may not be available on all platforms, and not all implementations have the same behavior. This change is similar to dotnet/coreclr#8823.
jack-pappas
added a commit
to jack-pappas/visualfsharp
that referenced
this pull request
Feb 28, 2017
Ported majority of the functionality from build.cmd to build.sh to make it easier to build/run the various test suites. The usual 'net40' projects build just fine, but some of the projects in the test suites seem to have build errors unrelated to build.sh. Accept but ignore 'ci' target for now. The mono distribution for Linux doesn't seem to include nuget as it does on macOS. Try to use built-in nuget but fall back to the binary in the repo if necessary. Use the POSIX built-in 'command' instead of 'which'; the latter may not be available on all platforms, and not all implementations have the same behavior. (See dotnet/coreclr#8823 for a similar change.)
jack-pappas
added a commit
to jack-pappas/visualfsharp
that referenced
this pull request
Feb 28, 2017
Ported majority of the functionality from build.cmd to build.sh to make it easier to build/run the various test suites. The usual 'net40' projects build just fine, but some of the projects in the test suites seem to have build errors unrelated to build.sh. Accept but ignore 'ci' target for now. The mono distribution for Linux doesn't seem to include nuget as it does on macOS. Try to use built-in nuget but fall back to the binary in the repo if necessary. Use the POSIX built-in 'command' instead of 'which'; the latter may not be available on all platforms, and not all implementations have the same behavior. (See dotnet/coreclr#8823 for a similar change.)
KevinRansom
pushed a commit
to dotnet/fsharp
that referenced
this pull request
Feb 28, 2017
Ported majority of the functionality from build.cmd to build.sh to make it easier to build/run the various test suites. The usual 'net40' projects build just fine, but some of the projects in the test suites seem to have build errors unrelated to build.sh. Accept but ignore 'ci' target for now. The mono distribution for Linux doesn't seem to include nuget as it does on macOS. Try to use built-in nuget but fall back to the binary in the repo if necessary. Use the POSIX built-in 'command' instead of 'which'; the latter may not be available on all platforms, and not all implementations have the same behavior. (See dotnet/coreclr#8823 for a similar change.)
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
) GNU which might not be installed on a minimal GNU/Linux installation. Refs dotnet/coreclr#6994 and dotnet/coreclr#7025. Commit migrated from dotnet/coreclr@7f8557f
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GNU
whichmight not be installed on a minimal GNU/Linux installation.It's also not listed in the building instructions
Build failed on my Arch Linux base install without error messages (
stderris redirected to/dev/null) because of the missing GNUwhichcommand even thoughpythonis installed:There is no need to use
whichbecause every POSIX shell provides command to get the pathname of a command.I know there was already a try and revert
I don't know why it was reverted/broke the build. Maybe because the original author @adityamandaleeka used
hashwhich has a different purpose (caching/manipulating command locations).