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

Put lib/vendor close to the webapp #2428

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

vmcj
Copy link
Member

@vmcj vmcj commented Mar 24, 2024

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.

@vmcj vmcj requested review from thijskh and eldering March 24, 2024 17:54
@thijskh
Copy link
Member

thijskh commented Mar 25, 2024

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
Copy link
Member Author

vmcj commented Mar 25, 2024

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
Copy link
Member

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
Copy link
Member

thijskh commented Mar 26, 2024

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

@nickygerritsen
Copy link
Member

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
Copy link
Member Author

vmcj commented Mar 26, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

vmcj commented Mar 31, 2024

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
Copy link
Member

thijskh commented Mar 31, 2024

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

@eldering
Copy link
Member

eldering commented Apr 9, 2024

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.

Default it would be put in the root (see also the commit message about
those lines in composer.json) and it makes the most sense to have those
relative to the source.

@nickygerritsen suggested another place which would also be fine, we
should discuss this before merging.
Also create a new cache as the old one should be quite out
of date and as we now move the vendor somewhere else this
is the right time to update the naming.
Added script for when we want to change this to another directory
@vmcj
Copy link
Member Author

vmcj commented Apr 27, 2024

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
Copy link
Member

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

@vmcj
Copy link
Member Author

vmcj commented Apr 27, 2024

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.

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.

None yet

4 participants