Skip to content

prevent bash process substitution error in cygwin#1650

Merged
thaJeztah merged 1 commit into
docker:masterfrom
MatteoOreficeIT:master
Feb 11, 2019
Merged

prevent bash process substitution error in cygwin#1650
thaJeztah merged 1 commit into
docker:masterfrom
MatteoOreficeIT:master

Conversation

@MatteoOreficeIT
Copy link
Copy Markdown
Contributor

Signed-off-by: Matteo Orefice matteo.orefice@bites4bits.software

fixes /dev/fd/62 : No such file or directory running cli docker bash completion in cygwin

I founded cygwin bash cannot handle bash Process Substitution in __docker_fetch_info() function

- What I did

Added code to run in cygwin doesn't use Process Substitution and fallbacks to here string

- How I did it

read -r client_experimental server_experimental server_os < <(__docker_q version -f '{{.Client.Experimental}} {{.Server.Experimental}} {{.Server.Os}}')

could be rewritten as here string avoiding a pipe creation :

read -r client_experimental server_experimental server_os <<< "$(__docker_q version -f '{{.Client.Experimental}} {{.Server.Experimental}} {{.Server.Os}}')"

- How to verify it

  1. Install cygwin
  2. Copy ./contrib/completion/bash/docker onto /etc/bash_completion.d/docker
  3. In bash prompt type docker and press multiple to trigger completion
  4. Check errors have not been shown like /dev/fd/62 : No such file or directory

- Description for the changelog
Prevent bash docker/cli completion error /dev/fd/62 : No such file or directory from occurring under cygwin

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 31, 2019

Codecov Report

Merging #1650 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #1650     +/-   ##
=========================================
+ Coverage   56.07%   56.17%   +0.1%     
=========================================
  Files         306      300      -6     
  Lines       20950    20640    -310     
=========================================
- Hits        11747    11594    -153     
+ Misses       8354     8210    -144     
+ Partials      849      836     -13

@albers
Copy link
Copy Markdown
Collaborator

albers commented Feb 3, 2019

Thanks for reporting. I have to take a closer look and hope to get back on you on monday.

@albers
Copy link
Copy Markdown
Collaborator

albers commented Feb 4, 2019

I cannot reproduce the problem.
I know that cygwin used to have problems with process substitution, and I remember having seen this error message several times. But current versions seem to cope well with process substitiution:

$ cat < <(echo Hugo)
Hugo

I tested with current cygwin 32-bit on Windows 7 and current cygwin 64-bit on Windows 10.
Maybe your installation is outdated?

Copy link
Copy Markdown
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

As your improved version also works for other operating systems, there's no need to branch here by OS.
Please change line 596 to always use command substitution.

@thaJeztah
Copy link
Copy Markdown
Member

@albers looks like the PR was updated

Comment thread contrib/completion/bash/docker Outdated
__docker_fetch_info() {
if [ -z "$info_fetched" ] ; then
read -r client_experimental server_experimental server_os < <(__docker_q version -f '{{.Client.Experimental}} {{.Server.Experimental}} {{.Server.Os}}')
# prevent bash substitution error in cygwin : /dev/fd/62 : No such file or directory error
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this comment here. It will not be useful for readers once this PR is merged.
It is sufficient to mention the rationale behind this change in the commit message.

Signed-off-by: Matteo Orefice <matteo.orefice@bites4bits.software>
@albers
Copy link
Copy Markdown
Collaborator

albers commented Feb 11, 2019

LGTM, thanks very much for your contribution.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!!

@thaJeztah thaJeztah merged commit 8271c94 into docker:master Feb 11, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Feb 11, 2019
@MatteoOreficeIT
Copy link
Copy Markdown
Contributor Author

Thanks to the docker community which allows us better devops, I hope to contribute many more time asap

@thaJeztah
Copy link
Copy Markdown
Member

Thanks! That's really appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants