add basic unit tests #837
add basic unit tests #837
Conversation
navi-desu
force-pushed
the
tests
branch
2 times, most recently
from
f5d974b to
725d92b
Compare
N-R-K
left a comment
N-R-K
left a comment
There was a problem hiding this comment.
I like tests. I don't like how testing specific hacks (--root, $RC_ROOT) are bleeding into regular builds though.
src/librc/librc.c
Outdated
| free(rc_dirs.svcdir); | ||
| for (size_t i = 0; i < ARRAY_SIZE(scriptdirs); i++) | ||
| if (rc_dirs.scriptdirs_data[i]) | ||
| free(rc_dirs.scriptdirs_data[i]); |
There was a problem hiding this comment.
Check is pretty useless since free(NULL) is no-op. I'd also set it back to NULL after free(), that's what the rest is doing.
src/librc/librc.c
Outdated
| { | ||
| int len = strlen(root); | ||
| if (root[len - 1] == '/') | ||
| len--; |
There was a problem hiding this comment.
Doesn't work for "/", besides, it's unnecessary anyways isn't it? Multiple slashes in path are redundant but don't cause any issue. But if you want to strip trailing slashes, then here's the proper way to do it (note len>1 rather then len>0):
while (len > 1 && root[len-1] == '/') len--;There was a problem hiding this comment.
it's unnecessary though i just find foo//nya ugly, and that would propagate to the symlinks created...
and i now i wonder if the symlinks (rc-update add, del) should be relative, since trying to use --root to build a chroot would give broken symlinks as is right now
src/openrc-run/openrc-run.c
Outdated
| const char *applet = NULL; | ||
| const char *extraopts = "stop | start | restart | status | describe | zap"; | ||
| const char getoptstring[] = "dDsSvl:Z" getoptstring_COMMON; | ||
| const char getoptstring[] = "dDsSvl:Z" getoptstring_DIRS getoptstring_COMMON; |
There was a problem hiding this comment.
Since --root is only for testing purposes, shouldn't we try to use something less invasive?
There was a problem hiding this comment.
it is not just for testing purposes, another use of it is to setup a chroot, image, etc, which is a feature i really missed while setting up my laptop and trying to build gentoo for arm
There was a problem hiding this comment.
Exactly. People have been wanting this for quite some time, even.
There was a problem hiding this comment.
Okay, that makes sense. But in that case, the commit messages should document it more clearly. Since the PR title was about adding tests, I assumed this would only be used for testing.
There was a problem hiding this comment.
i realized --root doesn't make much sense for rc-service (is there any valid case to start a service from a chroot... from outside it?)
maybe we keep RC_ROOT internally, but only expose --root for rc-update?
There was a problem hiding this comment.
I can think of some contrived cases for rc-status --root but they don't make much sense or are very unlikely, e.g. some /run inside the root that isn't a tmpfs.
navi-desu
force-pushed
the
tests
branch
3 times, most recently
from
a239c71 to
2b0a669
Compare
No description provided.