-
Notifications
You must be signed in to change notification settings - Fork 249
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
Keep the owner:group of the one downloading the tarball #2303
base: main
Are you sure you want to change the base?
Conversation
356fbe9
to
7296de4
Compare
7e5ff6e
to
d4432cd
Compare
d8656a5
to
33583cf
Compare
6b0880c
to
2e09c16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for a small comment.
webapp/Makefile
Outdated
@@ -12,16 +12,18 @@ SUBDIRS = config | |||
copy-bundle-assets: | |||
# We can not use bin/console here, as when using a fakeroot, | |||
# the include paths are broken. We just copy in the data we need | |||
-rm -rf public/bundles/nelmioapidoc | |||
-rm -rf public/bundles/nelmioapidoc/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: why not delete the whole directory? It gets recreated below, and this is/should be run as the normal user (because it's part of the domserver
build target), so it guarantees a clean copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried that but it doesn't work. I think because we have this in the main Makefile (at the root):
install-domserver: domserver composer-dump-autoload domserver-create-dirs
so we remove the folder again as super user (if you use sudo) and it would be owned by root:root
ls -atrl webapp/public/bundles/nelmioapidoc/
total 36
drwxr-xr-x 2 domjudge domjudge 4096 Jun 16 2023 swagger-ui
-rw-r--r-- 1 domjudge domjudge 6027 Jun 16 2023 style.css
-rw-r--r-- 1 domjudge domjudge 9212 Jun 16 2023 logo.png
-rw-r--r-- 1 domjudge domjudge 1628 Jun 16 2023 init-swagger-ui.js
drwxrwxrwx 3 domjudge domjudge 4096 Feb 18 00:10 ..
drwxr-xr-x 3 root root 4096 Feb 18 00:10 .
So the folder is now owned by root, but the files inside the dir by the current user (domjudge in my setup) or the user used for make domserver
. Not sure if we can solve this in a nice way or if the install-domserver
should not run domserver
at all?
Looking at: https://github.com/search?q=repo%3ADOMjudge%2Fdomjudge%20domserver%3A&type=code we use it mostly to make sure that make domserver
runs if the user forgot. Removing that first one seems to work but I'm not sure if this doesn't create other behaviour.
See DOMjudge#2303 (comment) for the reasoning behind this.
2e09c16
to
c7c5b81
Compare
Makefile
Outdated
@@ -47,7 +47,7 @@ endif | |||
domserver: domserver-configure paths.mk config | |||
judgehost: judgehost-configure paths.mk config | |||
docs: paths.mk config | |||
install-domserver: domserver composer-dump-autoload domserver-create-dirs | |||
install-domserver: composer-dump-autoload domserver-create-dirs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not remove this dependency of install-domserver
on domserver
. https://www.gnu.org/prep/standards/html_node/Standard-Targets.html shows that it is standard that an install target also compiles anything required. As also discussed in https://stackoverflow.com/questions/70901364/should-make-install-depend-on-compilation, I think that if you want/need to run make install
as root, then you should make sure to first run make build
(aka make domserver
) as yourself or accept that files are owned by root.
I guess this doesn't work because copy-bundle-assets
then gets executed again during domserver-install
, but that's something that should be fixed by making it a real dependency (not a "phony target"). That way, if it was already created during make domserver
, it won't get recreated during make domserver-install
with root ownership.
In the scenario: ./configure --prefix=<something_which_needs_su/sudo> make domserver <as unpriviliged user which downloaded the tarball> sudo make-installdomserver <as root> We would copy the Nelmiodoc files as root, so the user can't run a make {dist}clean without errors as we can't remove the folder as its owned by root. We now also install nelmiodoc together with composer in the base image for CI: - together with the needed dependencies - make sure we get an autoload.php
See DOMjudge#2303 (comment) for the reasoning behind this.
This makes that we don't rerun the rule during `domserver-install` if it already ran during `domserver`, so that if you install as root, the local files are still owned by the user who first ran `make domserver`, not root. This does assume that if the directory `public/bundles/nelmioapidoc` exists, then it contains the assets, and these won't automatically get updated, for example, in the setting of a maintainer-install.
c8b613d
to
570c856
Compare
@eldering I assume I had to re-add |
In the scenario:
We would copy the Nelmiodoc files as root, so the user can't run a
make {dist}clean
without errors as we can't remove the folder as its owned by root.