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.

Sign up for GitHub

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

Jump to bottom

Improve stdin parsing #1238

Merged
merged 7 commits into from
May 18, 2015
Merged

Improve stdin parsing #1238

merged 7 commits into from
May 18, 2015

Conversation

chriscool
Copy link
Contributor

This should fix issue #1141 (ipfs cat "multihash too short" error when using stdin).

@chriscool
parse_test: test multiline stdin
ff9cb9e 
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@chriscool
parse: don't use stdin if there are arguments
97ab64a 
This should fix issue #1141 (ipfs cat "multihash too short"
error when using stdin) and perhaps others.

License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@jbenet jbenet added the status/in-progress In progress label May 17, 2015
@chriscool
Copy link
Contributor Author

Some tests failed with:

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

and one with:

FAIL    github.com/ipfs/go-ipfs/routing/dht

@chriscool chriscool mentioned this pull request May 17, 2015
Closed
jbenet
jbenet reviewed May 17, 2015
View reviewed changes
commands/cli/parse.go
stringArgs, stdin, err = appendStdinAsString(stringArgs, stdin)
if err != nil {
return nil, nil, err
}
Copy link
Member

Choose a reason for hiding this comment

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

im not sure this works with all commands-- do we have a case where we use both arguments AND stdin? maybe some of the ipfs object put type things? (although those commands are not very intuitive right now, so perhaps they can change)

Copy link
Member

Choose a reason for hiding this comment

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

in general, this part of the cmdslib is a bit of a mess. ideas to clean it up would be great.

@chriscool
parse_test: improve error messages
5503c4c 
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@chriscool
parse: fix parsing stdin args
942e964 
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@chriscool
parse_test: add tests with 2 args and stdin
c93d583 
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@chriscool
parse: fix arg number check
53a80e2 
This should fix issue #1196 (Can't launch a command line
process from Qt).

The check was bad because it took stdin into account,
but it really shouldn't.

License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@jbenet
Copy link
Member

jbenet commented May 17, 2015

@whyrusleeping PTAL?

@chriscool
parse_test: test stdin enabled and not variadic arg
275ec7c 
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@chriscool
Copy link
Contributor Author

@jbenet @whyrusleeping I just added more improvements, tests and a fix for issue #1196 (Can't launch a command line process from Qt).
ipfs object put is a command like "stdinenablednotvariadic" in 275ec7c so there are now tests for commands like it too.
And there are now tests for commands than can take both an argument and stdin too (see tests for "stdinenabled2args" and "stdinenablednotvariadic2args").

@chriscool
Copy link
Contributor Author

The errors in Travis are the same "No output has been received in the last 10 minutes..."

@whyrusleeping
Copy link
Member

@chriscool this LGTM, those travis failures are strange, but not your fault.

@jbenet
Copy link
Member

jbenet commented May 18, 2015

rerunning---

We are using the KVM setup-- which is faster-- though is still beta.

@whyrusleeping
Copy link
Member

we should either set the go test timeout to 9 minutes, or the travis test timeout to 11 minutes, its hard to tell whether things broke because travis, or if things broke because one of our tests hung

@jbenet
Copy link
Member

jbenet commented May 18, 2015

@whyrusleeping sgtm

@chriscool
Copy link
Contributor Author

There are still the same kind of failures in some go tests:

*** Test killed with quit: ran too long (10m0s).
FAIL    github.com/ipfs/go-ipfs/merkledag   600.012s

and

--- FAIL: TestBootstrap (0.89s)
    dht_test.go:314: connecting 30 dhts in a ring
    dht_test.go:77: dial attempt failed: misdial to <peer.ID Y91TvF> through /ip4/127.0.0.1/tcp/41134 (got <peer.ID dTyTNY>)
18:47:07.334 CRITI   boguskey: TestBogusPrivateKey.Sign -- this better be a test! key.go:54
FAIL
FAIL    github.com/ipfs/go-ipfs/routing/dht 3.503s

whyrusleeping added a commit that referenced this pull request May 18, 2015
@whyrusleeping
Merge pull request #1238 from ipfs/improve-stdin-parsing
ae92955 
Improve stdin parsing
@whyrusleeping whyrusleeping merged commit ae92955 into master May 18, 2015
@whyrusleeping whyrusleeping removed the status/in-progress In progress label May 18, 2015
@jbenet jbenet deleted the improve-stdin-parsing branch May 18, 2015 20:14
This was referenced May 18, 2015
Closed
Closed
Merged
Closed
chriscool added a commit that referenced this pull request May 22, 2015
@chriscool
t0060: expect success with stdin
e7732ba 
This issue has been fixed by merging PR #1263 or PR #1238.

License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@chriscool chriscool mentioned this pull request May 22, 2015
Closed
chriscool added a commit that referenced this pull request May 22, 2015
@chriscool @jbenet
t0060: expect success with stdin
ffdbfe4 
This issue has been fixed by merging PR #1263 or PR #1238.

License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@wking wking mentioned this pull request Jun 12, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chriscool @jbenet @whyrusleeping