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

allow stdin on Windows #74

Merged
merged 1 commit into from
Apr 24, 2018
Merged

allow stdin on Windows #74

merged 1 commit into from
Apr 24, 2018

Conversation

djdv
Copy link
Contributor

@djdv djdv commented Mar 13, 2018

This may be all that has to be done in cmds for now, the rest may be in go-ipfs itself.
Ref: ipfs/kubo#4808

License: MIT
Signed-off-by: Dominic Della Valle <ddvpublic@gmail.com>
@codecov
Copy link

codecov bot commented Mar 13, 2018

Codecov Report

Merging #74 into master will increase coverage by 0.02%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   43.44%   43.47%   +0.02%     
==========================================
  Files          23       23              
  Lines        1908     1909       +1     
==========================================
+ Hits          829      830       +1     
  Misses        942      942              
  Partials      137      137
Impacted Files Coverage Δ
cli/parse.go 65.85% <33.33%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13acd1a...29d5b1e. Read the comment docs.

@djdv
Copy link
Contributor Author

djdv commented Mar 13, 2018

FWIW I plan on adding something similar to this in go-ipfs/shareness

  test_expect_success "stdin reading message shows up" '
  case $(uname) in
  MSYS*)
    test_expect_code 1 grep "ipfs: Reading from" add_help_err &&
    test_expect_code 1 grep "send Ctrl-z to stop." add_help_err
    ;;
  *)
    test_expect_code 1 grep "ipfs: Reading from" add_help_err &&
    test_expect_code 1 grep "send Ctrl-d to stop." add_help_err
    ;;
  esac
  '

which should address what codecov is upset over.

@Stebalien
Copy link
Member

This may be all that has to be done in cmds for now, the rest may be in go-ipfs itself.

Does this actually fix the issue in question or does something else need to be done to make stdin work on windows?

Do you have any idea why this check was added? I assume there was a reason.

@djdv
Copy link
Contributor Author

djdv commented Mar 13, 2018

Does this actually fix the issue in question or does something else need to be done to make stdin work on windows?

Partially, see ipfs/kubo#4808 (comment)
stdin+stdout appear to be working as long as the daemon is running, I'll have to investigate go-ipfs to see if something can be done about the lock while the daemon is down. I can't see why it's not released before the next command is issued.

I'm going to bounce back and forth trying to get tests around stdio working on Windows.

Do you have any idea why this check was added? I assume there was a reason.

It's a little unclear unfortunately.
ipfs/kubo#3266

@djdv djdv mentioned this pull request Apr 9, 2018
@whyrusleeping
Copy link
Member

@djdv any update here? Should we just merge this and push it into go-ipfs?

@djdv
Copy link
Contributor Author

djdv commented Apr 24, 2018

@whyrusleeping
I would say we merge this now and add handling in go-ipfs for the 1 case that doesn't work. (ipfs|ipfs with daemon down)

Edit: for reference, that issue is not Windows specific.

@djdv djdv requested a review from Stebalien April 24, 2018 15:17
@Stebalien
Copy link
Member

@whyrusleeping @djdv

Let's merge it. I still don't know why we disabled this command on Windows in the past (and not Linux) but we might as well try it.

@whyrusleeping whyrusleeping merged commit 368b806 into ipfs:master Apr 24, 2018
@djdv djdv mentioned this pull request Apr 25, 2018
9 tasks
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.

3 participants