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

Problem with setfacl #558

Closed
ss1h2a3tw opened this issue Apr 17, 2019 · 20 comments · Fixed by #559
Closed

Problem with setfacl #558

ss1h2a3tw opened this issue Apr 17, 2019 · 20 comments · Fixed by #559

Comments

@ss1h2a3tw
Copy link

Description of the problem / feature request

In latest version, the install script calls setfacl to set permission. Which is making packaging DOMjudge difficult in Debian/Ubuntu/Arch linux in my knowledge. Because while packaging it will call fakeroot to run install commands while pretending as root, but fakeroot does not support setfacl.
The permission in 6.0.3 is set by owners and groups, which seems working great for me. Can we change it back into the old way?

Your environment

commit: e5e8f3c
Distro: Arch linux

Steps to reproduce

./configure --with-webserver-group=http
make install-domserver while packaging

Expected behaviour

No errors.

Actual behaviour

setfacl:/home/username/domjudge/pkg/domjudge-domserver//opt/domjudge/domserver/webapp/var: Operation not supported

@nickygerritsen
Copy link
Member

We changed this because the eventdaemon was normally not run as the webserver user.
However today we got rid again of the eventdaemon, so we might be able to remove it again.

I'm currently not sure if 6.0.3 used the webserver user and group for var, because if it doesn't, we should fix that, as the webserver needs to write in this directory.

@nickygerritsen nickygerritsen added this to the 7.0 milestone Apr 17, 2019
@nickygerritsen
Copy link
Member

Update: seems we set the group to ${WEBSERVER_GROUP}, so it might indeed work without the setfacl now.
We will investigate 🙂

nickygerritsen added a commit that referenced this issue Apr 17, 2019
This was needed for the eventdaemon to work, but we dont have it anymore. Fixes #558.
@nickygerritsen
Copy link
Member

@ss1h2a3tw If you want, you can test the setfacl branch to see if it works. If so, can you check what the permissions of webapp/var are?

I can do this in a few hours after work but given that you have a checkout locally already that might be faster 😄 .

@ss1h2a3tw
Copy link
Author

OK, doing it now.

@ss1h2a3tw
Copy link
Author

With fhl enabled, the run is domjudge:root and submission is domjudge:http.
I will test with fhl disabled later.
But there are some bad news.
The autoload phps are using actual paths, which didn't change according to DESTDIR.
$baseDir = dirname('/home/username/domjudge/pkg/domjudge-domserver//usr/share/domjudge/webapp');

@nickygerritsen
Copy link
Member

Which autoload PHP is wrong?

sed -i "s#^\$$baseDir = .*#\$$baseDir = dirname('$(DESTDIR)$(domserver_webappdir)');#" $(1)/composer/$$file ; \
should fix all of them, so can you check if that is called during make install-domserver and make install-judgehost?

@ss1h2a3tw
Copy link
Author

Ahh.. I said it wrong. The baseDir should not contain the DESTDIR.

==> WARNING: Package contains reference to $pkgdir
usr/lib/domjudge/vendor/composer/autoload_files.php
usr/lib/domjudge/vendor/composer/autoload_namespaces.php
usr/lib/domjudge/vendor/composer/autoload_psr4.php
usr/lib/domjudge/vendor/composer/autoload_static.php
usr/lib/domjudge/vendor/composer/autoload_classmap.php

@ss1h2a3tw
Copy link
Author

ss1h2a3tw commented Apr 17, 2019

And here is the running command
for file in autoload_psr4.php autoload_classmap.php autoload_files.php autoload_namespaces.php ;
do sed -i "s#^\$baseDir = .*#\$baseDir = dirname('/home/shane/domjudge/domjudge/pkg/domjudge-domserver//opt/domserver/webapp');#" /home/shane/domjudge/domjudge/pkg/domjudge-domserver//opt/domserver/lib/vendor/composer/$file ;
done

@nickygerritsen
Copy link
Member

Ahh.. I said it wrong. The baseDir should not contain the DESTDIR.

==> WARNING: Package contains reference to $pkgdir
usr/lib/domjudge/vendor/composer/autoload_files.php
usr/lib/domjudge/vendor/composer/autoload_namespaces.php
usr/lib/domjudge/vendor/composer/autoload_psr4.php
usr/lib/domjudge/vendor/composer/autoload_static.php
usr/lib/domjudge/vendor/composer/autoload_classmap.php

Hmmm I thought the FHS setup would set DESTDIR to be empty. As in we use DESTDIR also to copy in the files, so I don't get why it contains your home dir. Is that because of fakeroot?
@eldering has more knowledge on this so I am adding him here. Perhaps we should add a check whether FHS is enabled and, if so, not include DESTDIR in the sed?

@ss1h2a3tw
Copy link
Author

For autoload_files.php autoload_namespaces.php autoload_psr4.php and autoload_classmap.php, the baseDir variable is wrong.
And for autoload_static.php, is looks like this:
public static $fallbackDirsPsr4 = array ( 0 => '/home/shane/domjudge/domjudge/pkg/domjudge-domserver//usr/share/domjudge/webapp/src', );

@ss1h2a3tw
Copy link
Author

ss1h2a3tw commented Apr 17, 2019

Yeah, I think so. Because when packaging it will often be a package's root directory like ./pkg/package-name. And call the Makefile to install the files into ./pkg/package-name/usr/ or something by passing the DESTDIR equals to ./pkg/package-name

Here is some old package script when 5.1.2 for example
https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=domjudge

@nickygerritsen
Copy link
Member

Just to doublecheck: you are trying to build a installable package, right? I think that is something we didn't test yet with the new FHS setup, we only tested running the FHS install directly, which will set DESTDIR to /, making it work. If that is the case I think indeed removing DESTDIR when FHS_ENABLED is true is the fix.

@ss1h2a3tw
Copy link
Author

This is the output for not enabling FHS

$ ls -al /opt/domserver
drwxr-xr-x 11 root root 4096 Apr 17 23:17 ./
drwxr-xr-x 4 root root 4096 Apr 17 23:17 ../
drwxr-xr-x 2 root root 4096 Apr 17 23:17 bin/
drwxr-xr-x 2 root root 4096 Apr 17 23:17 etc/
drwxr-xr-x 5 root root 4096 Apr 17 23:17 lib/
drwx------ 2 domjudge root 4096 Apr 17 22:51 log/
drwx------ 2 domjudge root 4096 Apr 17 22:51 run/
drwxr-xr-x 4 root root 4096 Apr 17 23:17 sql/
drwxrwx--- 2 domjudge http 4096 Apr 17 22:51 submissions/
drwxrwx--- 2 domjudge http 4096 Apr 17 22:51 tmp/
drwxr-xr-x 8 root root 4096 Apr 17 23:17 webapp/

@ss1h2a3tw
Copy link
Author

ss1h2a3tw commented Apr 17, 2019

Just to doublecheck: you are trying to build a installable package, right? I think that is something we didn't test yet with the new FHS setup, we only tested running the FHS install directly, which will set DESTDIR to /, making it work. If that is the case I think indeed removing DESTDIR when FHS_ENABLED is true is the fix.

Yeah, It is the case.

@nickygerritsen
Copy link
Member

This is the output for not enabling FHS

$ ls -al /opt/domserver
drwxr-xr-x 11 root root 4096 Apr 17 23:17 ./
drwxr-xr-x 4 root root 4096 Apr 17 23:17 ../
drwxr-xr-x 2 root root 4096 Apr 17 23:17 bin/
drwxr-xr-x 2 root root 4096 Apr 17 23:17 etc/
drwxr-xr-x 5 root root 4096 Apr 17 23:17 lib/
drwx------ 2 domjudge root 4096 Apr 17 22:51 log/
drwx------ 2 domjudge root 4096 Apr 17 22:51 run/
drwxr-xr-x 4 root root 4096 Apr 17 23:17 sql/
drwxrwx--- 2 domjudge http 4096 Apr 17 22:51 submissions/
drwxrwx--- 2 domjudge http 4096 Apr 17 22:51 tmp/
drwxr-xr-x 8 root root 4096 Apr 17 23:17 webapp/

Can you show webapp/var and its contents?

@ss1h2a3tw
Copy link
Author

Here it is.

total 56
drwxr-xr-x 5 root root 4096 Apr 17 23:17 ./
drwxr-xr-x 8 root root 4096 Apr 17 23:17 ../
drwxrwxr-x 2 domjudge http 4096 Apr 17 22:51 cache/
drwxrwxr-x 2 domjudge http 4096 Apr 17 22:51 logs/
drwxrwxr-x 2 domjudge http 4096 Apr 17 22:51 sessions/
-rw-r--r-- 1 domjudge http 33831 Apr 17 22:51 SymfonyRequirements.php

@nickygerritsen
Copy link
Member

That looks good 🎉. Let me fix the fhs stuff in a few hours and then I’ll merge

@ss1h2a3tw
Copy link
Author

OK, Thanks!

@nickygerritsen
Copy link
Member

I have just tested removing DESTDIR in FHS mode and that seems to go well 😄 .
Let's wait for @eldering to approve the PR and then I'll merge.

nickygerritsen added a commit that referenced this issue Apr 17, 2019
This was needed for the eventdaemon to work, but we dont have it anymore. Fixes #558.
@nickygerritsen
Copy link
Member

Thanks for reporting! It should be fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants