RFC: Pluggable Engines #2 (No Python, just evolution of current configure shell and make code) #375
Conversation
|
I like the fact that this approach uses the existing build system. However, it requires that each engine provides 3 files containing similar information. A more ideal approach IMHO would be to only require the configure file for each engine. Then, the script should be able to generate the current engines/engines.mk and engines/plugins_table.h automatically from the configure data. |
|
@bluegr: A fair point... I will look at modifying this to remove the engine-plugin.h files which should be fairly trivial. The engine.mk is a bit trickier due to sub engines, but perfectly achievable as the configure script already does most of that logic for outputting the enable/disabled engine status. |
|
@bluegr: There you go... Now only a configure file is required for each engine. |
|
I really like the changes in this pull request. As mentioned on -devel, I believe that the configuration script should be able to handle all engine plugins as black boxes, and read engine configuration data from them. As mentioned in -devel:
So, I'm all in for this solution, if it can be extended to handle engine authors. LordHoto's pull request #378 seems to handle the per-engine .po files. Also, if this approach is approved, we'll need to mirror these changes in create_project, too |
|
@bluegr : Thanks, but I would prefer to keep this PR limited to just moving the configure of engines and thus achieving the "plugable engines" aim i.e. just add the engine's folder or symlink, to a subdirectory of scummvm/engines and the engine is picked up by configure/make build system without further changes needed. I have an interest in this for a purely practical motive. I maintain a "Hydra" build locally of all the external engines symlinked and patching into the current git master in order to pick up symbol conflicts or changes in core code conflicts early enough to deal with them before we get issues when the engine is being prepared for inclusion in the main ScummVM repo later... hence my hope this will get merged to make this simpler for me. |
|
Name /configure is IMO misleading, since there is a chance someone would try to launch that, since it is a standard name. I would rather call it configure.in |
Since our current file containing this information is configure.engines, I would personally rather go with configure.engine. Especially since we use ".in" for files which are preprocessed by some other tool (i.e. update-version). |
Right, I just did not come up with a better name. |
|
@sev- : I doubt that the name would mislead as only the top level configure script would have execute permissions on a Unix system, and it is the top level... but I can see your point and I did consider this before. I also rejected "configure.in" as this is not preprocessed as @lordhoto has indicated... "configure.engine" seems a bit redundant in naming as it is the only configure in the engine's directory, but I too can't come up with a better name, so will commit to make this change... |
|
Do you need any help to look into adapting create_project to work with these changes? I think we should get this merged since it's definitly improved compared to the current state. |
|
@lordhoto : Yes, I could do with some help on that. I did think about regenerating engines/configure.engines as an interim step, but that would be a total hack... It should be fairly simple to change this, removing the parsing of configure.engines and replacing with an iteration over the engines/* directories looking for configure.engine and then parsing each in turn as per "for i in $_srcdir/engines/*/configure.engine;" but I admit I have not looked at the create_project code for this apart from a quick look to see if this was totally trivial to change. tl;dr YES PLEASE! :) |
|
I think we could consider merging this (after updating the pull request, it got rot) |
|
create_project will need to be updated accordingly before this is merged |
It should be farily simple, yes. The code for parsing configure.engines is in create_project.cpp lines 692-716. We have some function "listDirectory" which can list the contents of a directory. It should be trivial to adapt the code with that. I might be able to look into it this WE but I cannot promise that. |
|
I gave this pull request a try and noticed some issue whenever configure or the like changed. I am usually running "make -j5" and with this branch it runs configure twice (at the same time) whenever something related to configure changed. This is confusing. I'm not sure how to fix this since I'm not really familiar with Makefiles. Any ideas? EDIT: In fact even worse: It seems to run configure every time I run "make" (but then only once). |
|
I have made a typo fix (thanks to fuzzie for noticing) and a fix to prevent configure from being run on each make run over here: https://github.com/lordhoto/scummvm/commits/digitall-pluggable-engines This still does not fix that configure is run in parallel when a configure related file changed. This is caused by the rule in Makefile line 86. This rule does not mean that all the files on the left hand side are generated by a single command. Here is some information on how this can be done: https://www.gnu.org/software/automake/manual/html_node/Multiple-Outputs.html#Multiple-Outputs (thanks to wjp for digging this up). Here's some stackoverflow question on the same issue: http://stackoverflow.com/questions/2973445/gnu-makefile-rule-generating-a-few-targets-from-a-single-source-file |
|
I added another commit to the branch linked in my previous comment (i.e. https://github.com/lordhoto/scummvm/commits/digitall-pluggable-engines) which adapts create_project to parse configure.engine files and also generate engine_plugins_table.h. I was not able to test the resulting project files though. One possible issue could be that since engine_plugins_table.h is generated in the same directory as the project files the file is not found when compiling base/plugins.cpp. It would be cool if someone with MSVC could check up on this. |
|
Thanks for looking at this... I will try to update this PR and merge in your fixes tomorrow... then I'll look at fixing up the make code as per those references... (but far too tired currently..) |
|
Right. Have merged in @lordhoto 's changes and merged the latest master code in to allow the PR to be automatically and cleanly merged... Still need to look at the parallel make issue caused by the rule in Makefile line 86 ... |
|
Right... Have tried implementing the basic solution for multiple output files described in: Specifically: which has a number of bugs including the comment breaking the block i.e. it requires a \ on the end or removal... However, the result is not behaving as expected when the various output files are removed (I am running as make -j2 by default and the result is not good) to test... The issue seems to be that if I remove the "witness" file, this is fine, but removing any of the sub-generated files results in one make process ending in the configure/you need to configure section... I may be misreading this code as I'm not an expert on Makefiles, but I think we may have a circular dependency issue associated with the Makefile calling the configure ie. the code added in commit b820b66 when the makefile tries to recover from a "multilated" tree ie. when only some of the outputs are present... As far as I read this, this has worked reasonably for two outputs previously partly due to luck or rather as it was rare that config.h and config.mk were not generated together or one is removed and the scope for a race hazard in compilation was limited... but increasing the multiple outputs has made the -j2+ issue more likely to occur... Will have to think about this some more... Not sure if we have any developers more expert in Make code? Most of the changes to Makefile are by Max... but I see that @eriktorbjorn has done some work in Makefile.common, so any ideas? |
|
It's not for the best reason, but I would like it if engine_plugins_table.h was renamed or put outside of the root directory. (Reason: it interferes with tab completion of the engines/ directory... :-) ) |
|
Re. multiple-outputs: I think the way automake/autoconf gets around this is to have configure generate a script config.status that can generate the individual output files. (But I haven't thought this through 100%) |
|
The file you choose as a witness matters here, by the way. I think using a .mk file as the witness could cause part of the symptoms you describe. |
|
@wjp: It is a slightly spurious reason, but I am happy to oblige if it is possible. I don't really want to try moving it to a subdirectory (I don't want to foul up the code here and break "out of tree" builds), but this should be possible if really necessary... Renaming is a simpler solution. Can you suggest a good name which is similar... Maybe "engines_plugin_table.h" as that is more accurate i.e. we have a plural of engines, but only one plugin table... |
|
@wjp: Hmm... I have no idea about the automake/autoconf solution, but I have seen config.status files before... If you can provide a code example, then I may be able to implement... or if you have time... :) |
|
@wjp: Yes, I think you are right, the original was using config.h on the very left. I have swapped to using this as the witness and the behaviour is alot saner. I have committed this solution as 1daa582. @lordhoto : Could you test this please? I don't think it works if you remove engine_plugins_table.h manually and probably the other sub-targets ie. It fails to trigger the rule and call make on config.h to get the configure to automatically to regenerate... Not sure why, but I do think this fixes the parallel build otherwise... |
|
Hm, yes, it breaks with parallel builds because the engine_plugins_table.h rule removes config.h, so while it regenerates the former, anything using the latter can't be built, but make doesn't know about that. config.status is starting to sound quite attractive. The idea is to let configure not generate its outputs directly, but rather create a script that can create the outputs at will. When e.g. config.h is then missing, make can just call "config.status config.h" to have it regenerate config.h without re-running configure or touching any of the other outputs. (I'm not sure if this is the easiest solution at this point, but it may well be.) |
|
Hmm... Well, would be nice to know from @lordhoto if this solution has fixed the parallel build issue and thus this can get merged as "better than we have now" solution! :) But I tend to agree, I think this has been a latent issue which was not severe as only 2 files were output previously... I think config.status script output from configure sounds like a good idea and should be perfectly possible ie. the configure writes out the configuration into the top of config.status followed by a static block of code identical to that which is currently at the end of configure... then it calls this script to generate each of the outputs. This config.status then becomes the primary "witness" file for configure having been run... And as you describe, make can then call this config.status to generate any individual file if it is missing... or if config.status is missing, it triggers configure or prints the message as currently... I will look at implementing this solution in the next few days... but would still like to know if this is usuable as-is... |
|
Parallel builds seem to work fine now. However, I noticed another small difference to our master behavior: In master we always list SCUMM as first engine and had the remaining list alphabetically sorted in the "Engines" output of configure. With this branch the order seems to be completely alphabetical for me (also happens in create_project because I only sort them alphabetically there). |
|
@fingolfin : Thanks for this work. I will look at this.. Life is horribly busy IRL, so problematic to find time and energy. Probably at the weekend. I tend to agree with respect to the ordering, but it is probably best to keep these commits. The reasons being a) to ensure that we don't get any weird subtle regressions in the ports, or at least to avoid these until we have dealt with the initial impact and changes associated with this build system change, we should try to keep the outputs as close to the previous state as possible... though I draw the line at hacking the code to keep the misordering of MORTVIELLE and TSAGE, especially as that is an uintentional mistake. b) this was an intentional choice on the part of the team. I am happy that we change this, but I would like to do this in a explicit commit, rather than as a side effect of my changes, so it is clear and agreed. c) the code required in configure to deal with the scumm ordering is not very hacky, fairly small and self contained and introduced in 1 or 2 commits, so this can be easily reverted if/when we decide to change to pure alpha ordering of engines. |
|
Well, I personally don't mind the ordering, as you say it doesn't add much. Anyway, I hope this can be reviewed and merged before it bitrots again (e.g. when yet another engine is added). |
In fact it is already not mergable. But I took your rebased version and squashed it some more and made it apply cleanly to master again. The result can be found here: https://github.com/lordhoto/scummvm/tree/digitall-pluggable-engines-rebased / https://github.com/lordhoto/scummvm/compare/digitall-pluggable-engines-rebased Main changes:
If there are no objections to this rebased version, I want to merge this really soon (like tomorrow or later this evening if digitall is fine with the additional squashing). |
|
@lordhoto: I would prefer NOT to do the rebase into a single commit as you have lost a lot of information held in the commit messages of the squashed commits including the issues with parallel make... But my preferred solution would be fingolfin's rebased version is fixed to merge on top of the 7a16890 change and as long as the final reult matches the state of this branch here, I am happy. Sorry that I haven't done this already but I have had a number of personal issues IRL which have drained my time for this currently. I would prefer that you and fingolfin left this alone and I WILL do the rebase and cleanup on Sat for final review Sunday and merge. OK? |
|
If that information is important, it should be in comments in the code, not in commit messages. |
I thought I had also left the parallel make commit left alone and simply forgot to mention it (I definitly did do this in a first local test...) but it seems I accidently also squashed that one... I updated the branch on my repo to keep it as separate commit (which makes sense). Sorry for that. It should be fine now. I also compile tested all the commits again (both main code and devtools) and it works fine (minus parallel build issues in the first).
We can also do that. |
This is the first part of allowing engines to be added dynamically. They are placed into a folder in engines/ which must contain a file named "configure.engine" to add the engine, which is pulled into the top level configure script automatically.
This is the second part of allowing engines to be added dynamically. Each folder in engines/ which must contain a file named "engine.mk" containing the make definitions for that engine.
This is the third and final commit enabling fully pluggable engines. Now providing an engine folder contains a configure.engine, engine.mk and engine-plugin.h file, it will be picked up automatically by the configure script.
This is now generated automatically by the configure script from the engine directory names.
Each engine now only has to provide a single configure.engine file adding the engine into the configure script, which then produces the required other files automatically.
I could not try any generated project files since I do not have access to the IDEs.
This is due to the multiple outputs produced by the configure rule, which cause multiple invocations of configure when make is run in parallel. Various solutions are detailed in the Multiple-Outputs section of the GNU automake manual which apply generally to makefiles. This solution is a simpler one, but should solve the problem, though it can fail on "mutilated" trees ie. where some of the configure outputs are present, but not all... but this situation is not common, tends to be due to an error in configure and should be recoverable by a "make clean && ./configure" call.
This is mainly "cosmetic" to keep the SCUMM engine and subengines at the top of the various files, but probably a good idea to prevent any subtle regressions associated with changing the order.
This makes create_project output consistent with configure output again.
|
@wjp: Maybe, but that only can apply when there is a single change block in the commit where it is logical to add a TODO or similar. Sometimes a higher level comment is needed on the changeset... then I place these into the commit message which is logical as they are partly metadata, but that is a abstract argument and I would prefer to avoid further complication to this build change as it is already a PITA. @lordhoto, @SEV, @wjp, @fingolfin : Have reviewed fingolfin's rebase and changes and decided that this is much better so have migrated the PR branch to match his rebased version. The only minor change in the replacement AFAIK is in the configure test for missing engine directory which is now a "mkdir -p" and the result is cleaner code. I have considered Lordhoto's version which has further squashing, but would prefer to keep the smaller intermediate changesets of this version. As this has been a long PITA to get to this point and it is currently mergeable, I would like to get this reviewed and merged by Monday, so are the Core Team happy with this result? When you all indicate happy, I will merge or the last core team person can... |
|
The multiple splitting-out/removing commits seem like they might be annoying to bisect through, so it seems a pity not to squash those (but not a showstopper). In any case, please make sure to merge this (i.e. don't rebase and fast-forward the commits). |
|
This seems to work well enough for me. Ideally the commit history wouldn't move things around without directly putting them in their final location for something as fundamental as the build system, but I can live with it. (And similar for knowingly having out-of-tree builds broken for a few commits.) I haven't looked at the create_project changes at all; have those been tested by multiple people? |
I made Strangerke test the resulting files and it seems they are broken because the project file path isn't in the default include list. The fix I pushed over here lordhoto@11a8355 should fix that issue. So this commit should be appened before merging. (In case we squash the whole history more we can squash it into "DEVTOOLS: Adapt create_project for new configure.engine files."). |
|
@lordhoto : Appended cherry-picked commit lordhoto/scummvm@11a8355 ... |
|
It seems there's no objections to merge this, thus I'll do that now. |
RFC: Pluggable Engines #2 (No Python, just evolution of current configure shell and make code)
|
I also updated our Engines HOWTO and dropped a mail to -devel about this. Feel free to glance over those. Also since I forgot it in my previous comment: Thanks to everybody involved in this pull request. Great work! |
The following commits allow engines to be added to the build, just by dropping them into folders in engines/
This is achieved by the engine providing three files, configure, engine.mk and engine-plugin.h which each contain the engine relevant code/data previously found in engines/configure.engines, engines/engines.mk and engines/plugins_table.h respectively.
Modifications to the configure script and makefiles cope with scanning the engine/ directory for folders containing these and picking up their contents.
No further build dependency changes are needed, though create_project will need a minor modification to cope with the removal of engines/configure.engines and migration to the same kind of scan for engines/*/configure as the configure script now does...
This is functional, but I don't consider it final. I am open to modifications, naming changes and even if merged, my changes do not block us moving in the future to using CSV or JSON in the engine folders, though I haven't used either currently.