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

tests: Revert to using --owner-uid=$(id -u) #2423

Closed
wants to merge 6 commits into from

Conversation

cgwalters
Copy link
Member

This allows the test suite to run as non-root again. When operating
on a bare non-root repo, we can't chown().

(Arguably, non-root bare repos are not very useful and we shouldn't
test them or potentially even support them, but that's a whole
other thing)

@lucab
Copy link
Member

lucab commented Aug 26, 2021

As a non-root make check, locally for me this is a PASS either with or without of this patch. I'm thus unsure about the real backstory, but your explanation makes sense.
Perhaps we can directly avoid all issues by using a less intrusive flag which still enables the commit modifier, like --mode-ro-executables.

@cgwalters
Copy link
Member Author

As a non-root make check, locally for me this is a PASS either with or without of this patch.

I don't understand how that's possible. What is your build environment/setup, specifically? For me it's running in toolbox.

@lucab
Copy link
Member

lucab commented Aug 26, 2021

Nothing fancy, a Debian 11/stable workstation and a boring interactive user.
But it seems to also be fine in a bare debian:stable container and a fresh unprivileged user:

[core@coreos-scratch ~]$ podman run --rm -ti debian:stable
[... installing libs/git/automake ...]
root@6bf8b85b3c03:/# useradd -m devuser
root@6bf8b85b3c03:/# su - devuser
devuser@6bf8b85b3c03:~$ id       
uid=1000(devuser) gid=1000(devuser) groups=1000(devuser)
devuser@6bf8b85b3c03:~$ pwd
/home/devuser
devuser@6bf8b85b3c03:~$ git clone --depth 1 https://github.com/ostreedev/ostree.git && cd ostree && ./autogen.sh && make -j && make check
[...]
PASS: tests/test-basic-user.sh 36 commit from ref with modifier
[...]
PASS: tests/test-basic-user-only.sh 36 commit from ref with modifier
[...]
============================================================================
Testsuite summary for libostree 2021.4
============================================================================
# TOTAL: 875
# PASS:  832
# SKIP:  43
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

@cgwalters
Copy link
Member Author

It's test-basic.sh failing for me. In this case we represent uids just as they are on disk, so it cannot work to do --owner-uid=0 as non-root by default.

But clearly, something apparently did work in #2424 and I believe you it works locally.

One thing I'm narrowing in on is that I am passing through /var/tmp from the host so I get xattr support.

@cgwalters
Copy link
Member Author

One thing I'm narrowing in on is that I am passing through /var/tmp from the host so I get xattr support.

Yup from the CI logs:
SKIP: tests/test-basic.sh - this test requires xattr support

@cgwalters cgwalters marked this pull request as draft August 26, 2021 16:58
@cgwalters
Copy link
Member Author

cgwalters commented Aug 26, 2021

OK there's more here to do; marking draft.

@cgwalters
Copy link
Member Author

OK yeah. A lot of history here.
654b0c4
for example.

@cgwalters cgwalters marked this pull request as ready for review August 26, 2021 18:35
Followup to PRs related to ostreedev#2410

Since the test suite now covers this the test was failing on
a Fedora SELinux enabled host where we see `security.selinux`
even if not in the commit.
Hit this while working on some Rust code.
This is really the standard best practice, matching how
e.g. dpkg/rpm work, as well as most local development
environments (including mine) with e.g. `toolbox`.
This allows the test suite to run as non-root again.  When operating
on a bare non-root repo, we can't `chown()`.

(Arguably, non-root `bare` repos are not very useful and we shouldn't
 test them or potentially even support them, but that's a whole
 other thing)
It cannot work to use `--no-xattrs` when SELinux is enabled
because we get a `security.selinux` attribute on created files
regardless.  So just skip this test if true.

Also add some `ostree fsck`s in here which helped me debug
this.
I was seeing an `EPERM`  here which was confusing.
It turned out the real error was `EEXIST`.

Since we're referring to the original error, but we do a
lot of computation in the middle, we need to save errno.
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.

2 participants