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

exec: Execute batches before they get too long #1020

Merged
merged 1 commit into from
May 28, 2022

Conversation

tavianator
Copy link
Collaborator

Fixes #410.

@tavianator
Copy link
Collaborator Author

Based on sharkdp/argmax#5.

Currently some tests fail because they expect the arguments to be sorted. That seems hard to do. If you're okay with it, I'll fix the tests to ignore the order.

src/exec/mod.rs Outdated Show resolved Hide resolved
@tavianator
Copy link
Collaborator Author

tavianator commented May 25, 2022

Hmm, all the runners are failing with

/home/runner/.cargo/bin/cargo clippy --locked --all-targets --all-features
    Updating crates.io index
error: the lock file /home/runner/work/fd/fd/Cargo.lock needs to be updated but --locked was passed to prevent this
Error: If you want to try to generate the lock file without accessing the network, remove the --locked flag and use --offline instead.
Error: The process '/home/runner/.cargo/bin/cargo' failed with exit code 101

I thought it was due to the git dependency on argmax, but I switched to the released version and it's still failing. It works locally though:

$ cargo clippy --locked --all-targets --all-features
...
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s

Any ideas? I'll try cargo update I guess (no dice)

@tavianator
Copy link
Collaborator Author

I even tried running the exact workflow locally with https://github.com/nektos/act and it passed. Very confused.

@tavianator tavianator marked this pull request as draft May 25, 2022 16:51
@tavianator
Copy link
Collaborator Author

https://github.com/sharkdp/fd/runs/6596619198?check_suite_focus=true#step:5:1

I think I know what's happening: CI is testing the merge with master, not the branch itself. 🤦

@tavianator tavianator marked this pull request as ready for review May 25, 2022 17:00
Comment on lines -1511 to -1514
te.assert_output(
&["foo", "--batch-size", "0", "--exec-batch", "echo", "{}"],
"./a.foo ./one/b.foo ./one/two/C.Foo2 ./one/two/c.foo ./one/two/three/d.foo ./one/two/three/directory_foo",
);
Copy link
Owner

Choose a reason for hiding this comment

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

Why was this removed? Because it could fail on a hypothetical platform with a very low argmax limit?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah no, because it makes assumptions about the sort order. Ok. I guess it's not really worth the effort. This path is already tested sufficiently in other tests.

Comment on lines +184 to +189
if !self
.cmd
.args_would_fit(iter::once(&arg).chain(&self.post_args))
{
self.finish()?;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Very nice ❤️

@sharkdp
Copy link
Owner

sharkdp commented May 28, 2022

Really clear implementation. I also performed some successful local tests. No questions left 😄.

@sharkdp sharkdp merged commit 40b368e into sharkdp:master May 28, 2022
@tavianator tavianator deleted the fix-e2big branch May 28, 2022 20:20
@tmccombs
Copy link
Collaborator

tmccombs commented May 30, 2022

Sorry I didn't get to reviewing this sooner, but nice work @tavianator

@tavianator
Copy link
Collaborator Author

Thanks @tmccombs!

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.

-X should batch the number of passed files to the maximum supported by the shell
3 participants