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

wait for processes by their name #4849

Merged
merged 6 commits into from May 11, 2018
Merged

Conversation

s417-lama
Copy link
Contributor

Implement #4848 and refactored the code in wait command.

We can now specify the process name and wait for jobs with its name, like:

$ for i in (seq); sleep 10 &; end
$ wait sleep

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

- You can now specify the process name instead of process ids and wait for jobs.
- Refactor the code of wait command
@ridiculousfish
Copy link
Member

Nice tests! Can you please update doc_src/wait.txt to reflect these changes?

@s417-lama
Copy link
Contributor Author

I reflected the changes to doc_src/wait.txt :)

@s417-lama
Copy link
Contributor Author

s417-lama commented Mar 25, 2018

Although I committed the fix for the bug, I think I misunderstood the error in test.

I expect the behaviour like below:

$ for i in (seq 2 4); ls | sleep $i | cat > /dev/null &; end
$ sleep 3 | cat &
$ sleep 1 &
$ wait -n cat
$ jobs | wc -l
3

However, the failed test seems to behave

$ for i in (seq 2 4); ls | sleep $i | cat > /dev/null &; end
$ sleep 3 | cat &
$ sleep 1 &
$ wait -n cat
$ jobs | wc -l
2

Was one of the jobs finished immediately?
I tried many times, but I cannot reproduce the error in my local environment.

In the log file in failed test,

Job 4, 'sleep 1 &' has ended

is printed, but sleep 1 & should be Job 5, not Job 4... :(

@s417-lama
Copy link
Contributor Author

This might be related to #4778 or #4540

@mqudsi
Copy link
Contributor

mqudsi commented Mar 30, 2018

If you run jobs after each line you can see what's what.

@s417-lama
Copy link
Contributor Author

I cannot reproduce the bug although I tried the test many times :_(

After merging the current master branch I run the script below, but it never breaks in my environment.

set n 3
while [ $n = 3 ]
    for i in (seq 2 4); ls | sleep $i | cat > /dev/null &; end

    echo "jobs 1."
    jobs

    sleep 3 | cat &

    echo "jobs 2."
    jobs

    sleep 1 &

    echo "jobs 3."
    jobs

    wait -n cat
    set n (jobs | wc -l)

    echo "jobs 4."
    jobs

    wait

    echo "---"
end

How do we deal with this bug?

@faho faho added this to the fish-3.0 milestone May 11, 2018
@faho
Copy link
Member

faho commented May 11, 2018

Urgh... Sorry, I missed a change in the tests, which is why they are now failing!

Readding that line!

@faho faho merged commit 7c5297e into fish-shell:master May 11, 2018
@mqudsi
Copy link
Contributor

mqudsi commented May 11, 2018

I think waiting for processes by name is messy and brittle and job ids should always be used.

mqudsi added a commit that referenced this pull request May 11, 2018
Introduced by #4849 (add wait for processes by name)

../src/builtin_wait.cpp:23:14: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
    while (j = jobs.next()) {
           ~~^~~~~~~~~~~~~
../src/builtin_wait.cpp:23:14: note: place parentheses around the assignment to silence this warning
    while (j = jobs.next()) {
             ^
           (              )
../src/builtin_wait.cpp:23:14: note: use '==' to turn this assignment into an equality comparison
    while (j = jobs.next()) {
             ^
             ==
1 warning generated.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants