DOMjudge / domjudge

DOMjudge programming contest jury system
https://www.domjudge.org
GNU General Public License v2.0
703 stars 249 forks source link

Put `lib/vendor` close to the webapp #2428

Closed vmcj closed 2 days ago

vmcj commented 3 months ago

The files have nothing to do with binaries on the system but are only relevant for executable code from DOMjudge (which has no clearly defined place in FHS).

This was discussed in a private channel with the maintainers: https://domjudge-org.slack.com/archives/GJ2JX7B7X/p1709760201078489?thread_ts=1709540681.476949&cid=GJ2JX7B7X

The configure file is not cleaned up yet to clearly show this choice in the next minor release. If there is no new discussion this can be changed (as in removed) in the next major release.

I checked in a local container and it seems to work but I rather first discuss if this is the proper fix to handle this change in FHS.

thijskh commented 3 months ago

I agree with the change as it's an improvement. I do however think we should just flatten it further: I perceive no useful split between "libdir" and "datadir" as it's used currently in the domserver and I would just install all those things at the same root (which might be libdir).

The whole distinction between lib and share in FHS is dated and quickly becoming irrelevant (if it was ever relevant in the first place..), imho.

I think the RH effect of the webapp ending up in /run/webapp and other parts of the app under /usr shows to me what we currently do is not what I'd expect.

vmcj commented 3 months ago

I agree with the change as it's an improvement. I do however think we should just flatten it further: I perceive no useful split between "libdir" and "datadir" as it's used currently in the domserver and I would just install all those things at the same root (which might be libdir).

I'm fine with implementing but can you state where you want those things to endup (so quote me and just fill the right values)

#  * documentation.......: /usr/local/share/doc/domjudge
#  * domserver...........:
#     - bin..............: /usr/local/bin
#     - etc..............: /usr/local/etc/domjudge
#     - lib..............: /usr/local/lib/domjudge
#     - libvendor........: /usr/local/share/domjudge/lib/vendor
#     - log..............: /usr/local/var/log/domjudge
#     - run..............: /usr/local/var/run/domjudge
#     - sql..............: /usr/local/share/domjudge/sql
#     - tmp..............: /tmp
#     - webapp...........: /usr/local/share/domjudge/webapp
#     - example_problems.: /usr/local/share/domjudge/example_problems
#     - database_dumps...: /usr/local/var/lib/domjudge/db-dumps
eldering commented 3 months ago

Either way: we must make sure that the system works with any randomly set path for paths which are configurable. So if someone configures libvendordir and webappdir like this (as is currently possible):

#  * domserver...........:
#     - lib..............: /usr/local/lib/domjudge
#     - libvendor........: /my/weird/directory/lib/vendor
#     - webapp...........: /random/other/dir/share/domjudge/webapp

then that must work, or it must not be possible to configure these paths.

If I have some time, I'm actually planning to write some build tests, that check this among other things.

thijskh commented 3 months ago

Proposal: remove libvendordir setting and put it in webapp/vendor

nickygerritsen commented 3 months ago

Proposal: remove libvendordir setting and put it in webapp/vendor

This would also remove a customization we need to do after every Symfony upgrade, so I would be in favor.

vmcj commented 3 months ago

then that must work, or it must not be possible to configure these paths.

I propose to go for that 2nd option, I think spending your developer time on this (as you're the expert on this together with @thijskh IMHO) is not worth it. I personally prefer that that time is spend on other code (as I don't really see the usecase to split here).

Probably this breaks the CI still as @nickygerritsen mentioned a file which also needs changes.

nickygerritsen commented 3 months ago

If we do this, I would just move the thing alltogether, also in composer.json for example. Then libvendor can be completely removed from any configure scripts or makefiles, since it is already in the right place.

eldering commented 3 months ago

If we do this, I would just move the thing alltogether, also in composer.json for example. Then libvendor can be completely removed from any configure scripts or makefiles, since it is already in the right place.

Ok. I had the impression that in the past there were some other programs depending on this, but if that's not the case anymore, then sure.

vmcj commented 3 months ago

If we do this, I would just move the thing alltogether, also in composer.json for example. Then libvendor can be completely removed from any configure scripts or makefiles, since it is already in the right place.

Ok. I had the impression that in the past there were some other programs depending on this, but if that's not the case anymore, then sure.

So you're also fine with just removing this completely from configure.ac? It would be much simpler in that case if we don't display it there at all but that might be BC for people who had the opinion that those vendor files should be somewhere else.

thijskh commented 3 months ago

It seems highly unlikely to me that there's any deployer counting on this being configurable.

eldering commented 3 months ago

If we do this, I would just move the thing alltogether, also in composer.json for example. Then libvendor can be completely removed from any configure scripts or makefiles, since it is already in the right place.

Ok. I had the impression that in the past there were some other programs depending on this, but if that's not the case anymore, then sure.

So you're also fine with just removing this completely from configure.ac? It would be much simpler in that case if we don't display it there at all but that might be BC for people who had the opinion that those vendor files should be somewhere else.

Yes, I think we must then remove it from configure.ac: if it were still possible to modify there, then things would break.

vmcj commented 2 months ago

If we do this, I would just move the thing alltogether, also in composer.json for example. Then libvendor can be completely removed from any configure scripts or makefiles, since it is already in the right place.

@nickygerritsen it seems that it would put it next to webapp, not inside webapp. So we keep the problem for the copying for everything in webapp/public, did I do something wrong or are those indeed the proper locations (/vendor & /webapp)?

nickygerritsen commented 2 months ago

Hmmm maybe we should move the composer.json to webapp as well? Then it should be standard again

vmcj commented 2 months ago

Hmmm maybe we should move the composer.json to webapp as well? Then it should be standard again

That would fix 1 issue yes, not sure if it introduces other ones.