i was looking at the code and the ruleset compat argument is a bool, does that mean that freeciv supports only compatibility with the last version? if not, how is determined the method to load and other things, do all files have a format specification which freeciv looks at to know how to load?
Yes, even in the compat mode only the previous format version is supported. The compat mode exist only for updating the ruleset format (so we can load the ruleset to update), and user is expected to do it format version by format version (which is; once ever couple of years).
However, that boolean compat does *not* indicate that you are loading old version ruleset, merely that you are allowed to. The ruleset being loaded most likely still is a current format one; check also if loaded ruleset "compat->version < RSFORMAT_3_2" - there's many examples of this in ruleset.c (even more in S3_1 branch, where the comparison of course is against RSFORMAT_3_1)
It shouldn't have any problems but make sure to look over. If you find a problem should i do a interactive rebase and then send the new commits or just add more commits?
We want one commit ( / branch) / ticket.
0001-added-dummy-actions.ruleset-to-all-rulesets.patch
- Add the standard comment of the supplied ruleset files on top
- Set the descriptions correctly (only civ2civ3 one is part of civ2civ3, and they are "actions" data, "buildings" data)
For the code parts, read the doc/CodingStyle, and follow it.
For the ruleset.c part, in this ticket you should just openload_ruleset_file() actions.ruleset, and even that only if the other rulesets already indicate that this is a 3.2 ruleset (compat->version >= RSFORMAT_3_2) OR you're not loading in compat mode at all (treat any ruleset as it was a 3.2 one, and do the error checking based on that).
I'll try to add more steps to the #41572 metaticket tonight.
Reply To cazfi
We want one commit ( / branch) / ticket. 0001-added-dummy-actions.ruleset-to-all-rulesets.patch
- Add the standard comment of the supplied ruleset files on top
- Set the descriptions correctly (only civ2civ3 one is part of civ2civ3, and they are "actions" data, "buildings" data)
For the code parts, read the doc/CodingStyle, and follow it. For the ruleset.c part, in this ticket you should just openload_ruleset_file() actions.ruleset, and even that only if the other rulesets already indicate that this is a 3.2 ruleset (compat->version >= RSFORMAT_3_2) OR you're not loading in compat mode at all (treat any ruleset as it was a 3.2 one, and do the error checking based on that). I'll try to add more steps to the #41572 metaticket tonight.
Wow , i forgot to change the datafile.description field. About the coding style comment, are you mentioning it specifically because i did something wrong or only in general? i read the coding style file and it doesn't seem to have anything wrong, My lines are shorter than 77 characters, i am using 2 spaces indentation, i am separating the keywords with spaces, so if there is something i am forgetting please mention it.
About the changes in server/ruleset.c is it a problem or can i just leave it as is? i almost certainly will use the function i created and i added a description for it,
About the if, analyzing it you can see that it checks the version of the format, and then if it is lower than the RSFORMAT_3_2 it checks if we are in compatibility mode, if yes it calls the load_ruleset _actions with the gamefile, if not it errors, if the version is equal or higher than RSFORMAT_3_2 it opens the actionfile checks if it is open then if so it it calls load_ruleset_actions. the idea is that i will move the action related code to load_actions_ruleset with the same name, and have it accept a compat info, so that it could load action related info from the game file.
just to be sure only one commit per ticket? personally i like to have multiple commits, to do only one commit, i think i will normally do my commits and them use a interactive rebase to squash them into one.
Of course I have nothing against you having multiple commits in your local tree, before squashing them to a patch to submit. Just there generally should be just one commit (/branch) to the freeciv repo from each ticket. However, if you want to develop something in multiple commits, you should think if it indicates that there should actually be multiple tickets too.
The ruleset.c change is a bit different from what I would have done myself, but I can't tell any specific problem that your way would cause -> seem acceptable. The function stub added there seems like start of #45039
Some style issues:
void func (int parameter) {
void func (int parameter)
{
}
else {
->
} else {
Add empty line between variable declarations and actual code.
Declaration of load_ruleset_actions() has the second parameter (on its own line) badly intended
Missing spaces beween parameters
- load_ruleset_actions(actionfile,&compat_info);
- fc_snprintf(filename,sizeof(filename), "%s/actions.ruleset",path);
Reply To cazfi
Of course I have nothing against you having multiple commits in your local tree, before squashing them to a patch to submit. Just there generally should be just one commit (/branch) to the freeciv repo from each ticket. However, if you want to develop something in multiple commits, you should think if it indicates that there should actually be multiple tickets too. The ruleset.c change is a bit different from what I would have done myself, but I can't tell any specific problem that your way would cause -> seem acceptable. The function stub added there seems like start of #45039 Some style issues: --- void func (int parameter) { -> void func (int parameter)
{ --- }
else {
-> } else { ---- Add empty line between variable declarations and actual code. --- Declaration of load_ruleset_actions() has the second parameter (on its own line) badly intended --- Missing spaces beween parameters - load_ruleset_actions(actionfile,&compat_info);
- fc_snprintf(filename,sizeof(filename), "%s/actions.ruleset",path);
ok, i am not detail oriented enough to catch all these minor mistakes by myself, do you have a linter already configured which you can pass me the config or will i need to find one and configure it? theses types of mistakes can be caught and fixed by a linter. as i am in the topic of development tools what do you use? i am using neovim with clangd as a language server and i really want to add .cache and compiler_commands.json to the .gitignore so that i can just go back to doing "git add ." can i do that?
Reply To dark-ether
ok, i am not detail oriented enough to catch all these minor mistakes by myself, do you have a linter already configured which you can pass me the config or will i need to find one and configure it? theses types of mistakes can be caught and fixed by a linter. as i am in the topic of development tools what do you use? i am using neovim with clangd as a language server and i really want to add .cache and compiler_commands.json to the .gitignore so that i can just go back to doing "git add ." can i do that?
I don't think we have any linter config around (at least recent one) -> you may want to create a new ticket about adding one, e.g., under scripts/ , and to have general discussion about it.
Those additions to .gitignore -> new ticket
Are you using separate build director(y/ies), or have you configured the source directory? Just wondering if those files really get added to the source directory, or are they added to the build directory which happens to be same as source directory in your case. For a number of reasons I recommend using separate build directories.
Reply To cazfi
Reply To dark-ether
ok, i am not detail oriented enough to catch all these minor mistakes by myself, do you have a linter already configured which you can pass me the config or will i need to find one and configure it? theses types of mistakes can be caught and fixed by a linter. as i am in the topic of development tools what do you use? i am using neovim with clangd as a language server and i really want to add .cache and compiler_commands.json to the .gitignore so that i can just go back to doing "git add ." can i do that?
I don't think we have any linter config around (at least recent one) -> you may want to create a new ticket about adding one, e.g., under scripts/ , and to have general discussion about it. Those additions to .gitignore -> new ticket Are you using separate build director(y/ies), or have you configured the source directory? Just wondering if those files really get added to the source directory, or are they added to the build directory which happens to be same as source directory in your case. For a number of reasons I recommend using separate build directories.
how do i configure separate build directories and why do you recommend to do so? i am almost certain it would generate the files in the source directory but i may be wrong.
Reply To dark-ether
how do i configure separate build directories and why do you recommend to do so?
The main advantage is that you can have multiple build directories with different configurations. Probably the most typical use is to have separate build directories for building each client, though those multiple clients can be configured to a single configuration. If you want to test build against both Qt5 and Qt6, you have to have separate configurations for them (can't have --with-qtver=qt5 and --with-qtver=qt6 simultaneously). Same if you want to test build with multiple compilers.
It's as simple as that "working directory when you run 'configure'" is the build directory. So if you have sources under "src" and want "build" as a sibling directory, cd to "build" and run configure as "../src/configure". This is standard functionality of autotools, thought there are some projects with bugs in their bootstrap breaking it.
See also https://www.freelists.org/post/freeciv-dev/Working-with-multiple-branches-and-setups
you haven't mentioned any problems with the last version i sent (0001-add-dummy-actions.c-fixed-coding-style-violations.patch), when you have time please look it over.
Reply To dark-ether
you haven't mentioned any problems with the last version i sent (0001-add-dummy-actions.c-fixed-coding-style-violations.patch), when you have time please look it over.
Will check it shortly. In general, I might be a bit slow to reply for the rest of July - trying to have a bit of vacation.
save_actions_ruleset() : '{' to it's own line & add an empty line after declaring 'sfile'
Another thing I now noticed that you have "web-compatible" capability set for all actions.rulesets. Well, those empty files certainly have nothing web-breaking, but I'd say it's better to have the capability defined only in those rulesets where also the other .ruleset files do so.
should i delete the old not used patches files?
don't worry about taking too long, i have a billion unfinished projects which i can spend my time on while i wait for review.
git complained a couple of trailing whitespaces. Fixed in the attached version.
Splitting #41572.
Add the actions.ruleset, without any real content, just the fields required for all ruleset files ( [datafile] section )
- Add opening the file to ruleset.c - with the complication that it should not happen when loading freeciv-3.1 ruleset in compat-mode
- Add saving the file ( [datafile] section ) to rulesave.c
- Add the dummy file to all supplied rulesets
- Make the new files to be included in the distribution tarball, and to get installed in both autotools and meson based build (changes to Makefile.ams and meson.build)