-
Notifications
You must be signed in to change notification settings - Fork 262
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
Conversation
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 I think the RH effect of the webapp ending up in |
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)
|
Either way: we must make sure that the system works with any randomly set path for paths which are configurable. So if someone configures
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. |
Proposal: remove libvendordir setting and put it in |
This would also remove a customization we need to do after every Symfony upgrade, so I would be in favor. |
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. |
If we do this, I would just move the thing alltogether, also in |
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 |
It seems highly unlikely to me that there's any deployer counting on this being configurable. |
Yes, I think we must then remove it from |
@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 ( |
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. |
e2059da
to
61556d7
Compare
c5e6175
to
ef3a08a
Compare
45b231b
to
512e294
Compare
00c45ba
to
6d63100
Compare
This simplifies a couple of things. The Makefile targets are now moved to the webapp folder which also make the process easier to read IMO. The option to set the libvendor dir with configure has now also been removed as it lead to issues and we can't think of good usecases for them. Also moved the phpstan files to the webapp folder as we only test code from within that folder which simplifies the config. Also removed some unrelated unused MySQL debugging and refactored the code for the composer test after fixing the tests.
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.