clar: use internal functions instead of /bin/cp and /bin/rm #5528
Conversation
ethomson
force-pushed
the
ethomson/clar_internal
branch
from
467d2a9 to
fdfa742
Compare
There was a problem hiding this comment.
I had created a similar implementation multiple years ago which used our own internal futil helpers to do this, so I'm very much in favor of doing the conversion! Your approach definitely makes more sense, though, as it's also upstreamable.
There's a few small remarks, but overall this looks good to me!
| cl_assert(ret <= (ssize_t)len); | ||
| len -= ret; | ||
| } | ||
| cl_assert(ret >= 0); |
There was a problem hiding this comment.
Nit: this can be cl_assert(ret == 0)
There was a problem hiding this comment.
Writing the last chunk of the buffer will return a positive ret (indicating the number of bytes written). That will be subtracted from len, which will now be 0. In this case, ret will be positive but we should stop the loop successfully.
tests/clar/fs.h
Outdated
|
|
||
| close(in); | ||
| close(out); | ||
|
|
tests/clar/fs.h
Outdated
| const char *base; | ||
| int base_len; | ||
|
|
||
| /* Target exists; append basename */ |
There was a problem hiding this comment.
I think this is the case where we copy a file into a pre-existing directory, right? In that case, we should also assert S_ISDIR(dest_st.mode) so that one gets a proper error message if it isn't.
Ah, interesting. I always wondered where all those messages on macOS came from, but that makes a lot of sense to me! |
clar has historically shelled out to `/bin/cp` to copy test fixtures into a sandbox. This has two deficiencies: 1. It's slower than simply opening the source and destination and copying them in a read/write loop. On my Mac, the `/bin/cp` based approach takes ~2:40 for a full test pass. Using a read/write loop to copy the files ourselves takes ~1:50. 2. It's noisy. Since the leak detector follows fork/exec, we'll end up running the leak detector on `/bin/cp`. This would be fine, except that the leak detector spams the console on startup and shutdown, so it adds a _lot_ of additional information to the test runs that is useless. By not forking and using this internal system, we see much less output.
Similar to how clar has used `/bin/cp` to copy files, it's used `/bin/rm` to remove them. This has similar deficiencies; meaning that leaks is noisy and it's slow. Move it to an internal function.
ethomson
force-pushed
the
ethomson/clar_internal
branch
from
fdfa742 to
2a2c5b4
Compare
clar has historically shelled out to
/bin/cpto copy test fixtures into a sandbox and/bin/rmto clean them up. This has two deficiencies:/bin/cpis slower than simply opening the source and destination and copying them in a read/write loop. On my Mac, the/bin/cpbased approach takes ~2:40 for a full test pass. Using a read/write loop to copy the files ourselves takes ~1:50. Similarly,/bin/rmis slower than doing this internally. Moving to an internal traversal shaves another 20 seconds off.These numbers are less impressive on Linux, but using
sendfilethere makes a notable improvement.It's noisy. Since the leak detector follows fork/exec, we'll end up running the leak detector on
/bin/cpand/bin/rm. This would be fine, except that the leak detector spams the console on startup and shutdown, so it adds a lot of additional information to the test runs that is useless. By not forking and using this internal system, we see much less output.Since this repo actually has tests and fixtures, I'm opening this PR here and will port it to https://github.com/clar-test/clar once approved.