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

waiting for input contaminates buku output. #513

Closed
crocket opened this issue May 29, 2021 · 14 comments
Closed

waiting for input contaminates buku output. #513

crocket opened this issue May 29, 2021 · 14 comments

Comments

@crocket
Copy link

crocket commented May 29, 2021

My programs parse the output of buku. The programs are called by fzf on terminal emulator.
However, the output is often contaminated with waiting for input\n at the beginning.

fzf needs to reload the list of bookmarks by calling a script that updates buku cache by calling buku -p -f 5.

If fzf called the script after calling buku -w number on another terminal emulator, waiting for input\n contaminates the output and kills the script.

If fzf called the script without calling buku -w number, waiting for input\n doesn't contaminate the output.

Is print('waiting for input') really absolutely necessary for correct functionality?

@rachmadaniHaryono
Copy link
Collaborator

related to this issue qutebrowser/qutebrowser#2491

which come from this #447

@crocket
Copy link
Author

crocket commented May 29, 2021

If you delete print('waiting for input'), everything still should work fine.

It turns out that if fzf calls buku -p -f 5 directly, waiting for input comes out unconditionally.

My script doesn't call buku if it doesn't need to update buku cache, so the issue didn't surface until I called buku -w number. The script reads the cache instead of calling buku if cache is intact.

@crocket
Copy link
Author

crocket commented May 29, 2021

I mapped ctrl-r to reload(buku -p -f 5) in fzf. reload(buku -p -f 5) reads the output of buku -p -f 5 and uses it as new fzf entries.

@crocket
Copy link
Author

crocket commented May 30, 2021

qutebrowser/qutebrowser#2491 (comment) pointed out that not calling piped_input fixed the issue.

qutebrowser worked around piped_input by closing standard input.

#447 worked around print('waiting for input') by adding -j file option. Additional complexity was added in order to work around piped_input.

I think deleting print('waiting for input') can be a solution. If you don't need to read command line options from standard input, you can also delete piped_input. Reading command line options from standard input is quite a rare usecase that isn't worth breaking common use cases such as parsing the output of buku in programs or calling buku in programs that don't close standard input. I lost an entire day to finding the root cause of this issue.
The behavior of piped_input is also not documented by man buku or buku --help. I suggest deleting piped_input.

If I wanted to read command line options from standard input, I can easily write a shell script for that.

@jarun
Copy link
Owner

jarun commented May 30, 2021

Reading command line options from standard input is quite a rare usecase that isn't worth breaking common use cases

I see the following work if piped_input() is gone:

1 ~/GitHub/buku$ cat input                        
www.google.com
www.yahoo.com
1 ~/GitHub/buku$ cat script 
#!/usr/bin/env sh

while IFS= read -r url; do
    ./buku -a $url
done < input
1 ~/GitHub/buku$ xargs -I {} -a input ./buku -a {}
481. Google
...
482. Yahoo India | News, Finance, Cricket, Lifestyle and Entertainment
...

1 ~/GitHub/buku$ ./buku -d 481-482                
...
1 ~/GitHub/buku$ ./script                         
481. Google
...
482. Yahoo India | News, Finance, Cricket, Lifestyle and Entertainment
...

What don't work are:

1 ~/GitHub/buku$ cat | ./buku -a
usage: buku [OPTIONS] [KEYWORD [KEYWORD ...]]
buku: error: argument -a/--add: expected at least one argument
// WAITS HERE
1 ~/GitHub/buku$ cat input | ./buku -a
usage: buku [OPTIONS] [KEYWORD [KEYWORD ...]]
buku: error: argument -a/--add: expected at least one argument
1 ~/GitHub/buku$ echo "www.google.com" | ./buku -a
usage: buku [OPTIONS] [KEYWORD [KEYWORD ...]]
buku: error: argument -a/--add: expected at least one argument

How does your script call buku and what is the use case where there is no input?

The problem this isn't so straight is - many utilities use buku and they have adapted to it. Removing this may cause unforeseen issues.

However, I don't see a reason to wait for data if no arguments are passed. Can you try the following patch?

diff --git a/buku b/buku
index 38949fd..1d9a3e0 100755
--- a/buku
+++ b/buku
@@ -5043,10 +5043,11 @@ def main():
     pipeargs = []
     colorstr_env = os.getenv('BUKU_COLORS')
 
-    try:
-        piped_input(sys.argv, pipeargs)
-    except KeyboardInterrupt:
-        pass
+    if len(sys.argv) > 1:
+        try:
+            piped_input(sys.argv, pipeargs)
+        except KeyboardInterrupt:
+            pass
 
     # If piped input, set argument vector
     if pipeargs:

Note that we treat only-positional-argument implicitly as -s positional_arg but I guess it's easy to fix that issue from the caller's side by calling buku -s.

The following work with this:

$ echo "hello" | ./buku -s
$ cat input | ./buku -a  
waiting for input
481. Google
...

@crocket
Copy link
Author

crocket commented May 30, 2021

Removing this may cause unforeseen issues.

Reading command line arguments from standard input is an undocumented feature. Only documented public contracts should be preserved for some time. Various projects explicitly tell people to expect undocumented features to be broken at any point.

I discovered this feature only after reading source code. I read the source code because waiting for input\n was breaking my parsers. It was really difficult to diagnose where waiting for input\n was coming from. It took an entire day and a morning shower to figure out that buku may be the culprit.

Why would you expect people to rely on undocumented esoteric features? Just remove it. Retaining the feature already caused three people to file an issue. It wasted man hours. It can be implemented by a simple shell script which is usually written by a person who knows a system from inside out.

if len(sys.argv) > 1 would still break my use case because I parse standard output of buku -p 1 23 ... -f 20 and buku -p -f 5 in my scripts which are executed from fzf. Child processes may not necessarily have a tty as standard input.

@jarun
Copy link
Owner

jarun commented May 30, 2021

It was really difficult to diagnose where waiting for input\n was coming from. It took an entire day and a morning shower to figure out that buku may be the culprit.

Usually I grep the code of the utilities involved when I try something new and see an error message. I can understand it was unexpected and may get overwhelming when faced the first few times.

Why would you expect people to rely on undocumented esoteric features? Just remove it. Retaining the feature already caused three people to file an issue. It wasted many man hours of various people. qutebrowser. Me. Someone else. It can be implemented by a simple shell script which is usually written by a person who knows a system from inside out.

Sorry, I don't like your tone and it's not really the time I would want to engage in a war of words on a mundane python script.

You have to understand that I don't owe you or anyone else anything for using my utility.
I mentioned earlier I need to think about it. I have also spent my time in thinking about the issue you saw and trying different things.

But this thread doesn't read like a discussion anymore. It's more of an accusation now.

@jarun jarun closed this as completed May 30, 2021
@crocket
Copy link
Author

crocket commented May 30, 2021

I have been on a high-pressure project. While on a high-pressure project, I had to deal with this. Sorry for being angry at you.
But, you should at least consider my pull request instead of closing it out of bad emotions.

@crocket
Copy link
Author

crocket commented May 30, 2021

It should be possible to keep piped_input and fix this issue, too.

It's going to be your choice to do nothing out of anger or fix the issue which someone else who's not me will encounter someday.

@jarun
Copy link
Owner

jarun commented May 30, 2021

out of bad emotions
out of anger

I don't have any of those. These kind of discussions are cheap and lack maturity, that's all. I prefer to avoid these. Not exactly a great start to a bright Sunday morning one should spend with family.
When you are done with your project and things calm down, re-read this thread and hopefully you'll find many things wrong in the approach.

Coming to the tool issue,

How about adding an option to explicitly instruct buku to skip reading stdin? We can mandate it to be the first argument, if used. That way if we find a valid argv with a length and it's the new option, we skip reading stdin.

@crocket
Copy link
Author

crocket commented May 30, 2021

How about adding an option to explicitly instruct buku to skip reading stdin? We can mandate it to be the first argument, if used. That way if we find a valid argv with a length and it's the new option, we skip reading stdin.

That is okay, but it is also feasible to add an option to read standard input as command line options. I don't have a strong opnion about either option, but the thing about standard input shall be documented properly if you choose to add an option to ignore standard input instead of an option to honor standard input.

An option to treat standard input as command line arguments may minimize surprises experienced by new users. But, you can argue otherwise.

@jarun
Copy link
Owner

jarun commented May 30, 2021

I would say we should use the option to explicitly say - do not wait for stdin. Probably --nostdin.

And we should update the message to:

waiting for input (if this blocks, try --nostdin)\n

@jarun
Copy link
Owner

jarun commented May 30, 2021

Please update your PR accordingly.

jarun added a commit that referenced this issue May 30, 2021
No need to add the option to auto-completion scripts
as it does not apply to manual input from a tty.
@jarun
Copy link
Owner

jarun commented May 30, 2021

For the record (I couldn't remember this earlier), we do have a common use-case for reading from a non-tty:

https://github.com/jarun/buku/wiki/System-integration#bookmark-from-anywhere

Repository owner locked as resolved and limited conversation to collaborators May 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants