Forbid singlepole setting requirements unless alltemperate is disabled
alienvalkyrie Reply To alienvalkyrie
It is currently possible to have singlepole requirements without also checking that alltemperate is disabled. This is nonsensical, since when alltemperate is in fact enabled, singlepole should not make any difference. I can see a number of ways to deal with this: * Mark singlepole requirements deprecated unless (a) there is also a negated alltemperate requirement or (b) alltemperate is locked FALSE by the ruleset
* As above, but make it a hard ruleset loading failure
alienvalkyrie Quickie note on disjunctive requirement lists (which is only building obsoletion requirements, afaict): singlepole requirements for those are only valid iff (a) there is a present alltemperate requirement, or (b) alltemperate is locked FALSE by the ruleset. Since in the latter case, a present alltemperate requirement would always be inactive, i.e. have no effect on disjunctive requirement lists, we can drop (b) here as well and demand (a) without loss of generality.
(Granted, having a building that is always obsolete given certain server settings is unlikely to be a thing anyway, but y'know, let's keep it general here.)
alienvalkyrie alienvalkyrie Master patch is attached; S3_1 patch still to go.
Should we also do something for S3_0, e.g. mark those cases as deprecated, starting with 3.0.1? Get warning messages out to ruleset authors ASAP?
alienvalkyrie Reply To cazfi
S3_1 compat mode, loading S3_0 ruleset, should make sensible conversion
Since this is rscompat stuff for requirement vectors (which appear in multiple ruleset files), we might have to backport #43708 to S3_1. Alternatively, we could use the same solution/workaround as hrm #695469 originally used, i.e.
check all relevant versions and only apply when all of them are old
Reply To alienvalkyrie
Should we also do something for S3_0, e.g. mark those cases as deprecated
Yeah, deprecation warning would be the right thing to do, even if it seems that we still have work to do in educating ruleset authors to enable those warnings.
alienvalkyrie Reply To cazfi
Reply To alienvalkyrie
we might have to backport #43708 to S3_1
I'm not against.
Done #44203
alienvalkyrie alienvalkyrie Reply To cazfi
Reply To alienvalkyrie
Should we also do something for S3_0, e.g. mark those cases as deprecated
Yeah, deprecation warning would be the right thing to do, even if it seems that we still have work to do in educating ruleset authors to enable those warnings.
~> S3_0 patch attached
(I'm not retargetting this ticket to 3.0.1 or 3.0.2, since I feel it's more important that this is blocking S3_1 d3f)
alienvalkyrie alienvalkyrie S3_1 patch done. I'm not sure I'm happy with it – there is currently no place in the code with access to both rscompat info and whether a given requirement vector is conjunctive (both of which are relevant to this change). One option would've been to pass a bool conjunctive to lookup_req_list(), but that would've added a lot of small changes all over the place ~> higher chance for conflict with other current patches, and future S3_1 patches are less likely to also work for other branches. Given that this is all about (a) an unlikely edge case and (b) something where affected ruleset authors will have to check the result anyway, I used a heuristic instead – if the secfile key is "obsolete_by", we're disjunctive; otherwise, we're probably conjunctive. This matches current rscompat and ruleset load code.
If this heuristic ever fails (in a place where there is a singlepole requirement), it will make rscompat introduce an erroneous requirement and lead to ruleset load failure, which isn't great. The more advanced alternative would be to trust that any existing alltemperate requirement is the one we need (maybe print a warning if we think it should be flipped) and not change anything in those cases.
Well, luckily this code is only needed in 3.0 -> 3.1 conversion, and not in "future" where format might be different. We do know that in the 3.0 format buildings "obsolete_by" is the only disjunctive list.
alienvalkyrie alienvalkyrie Did a bit of testing and everything seems to work swimmingly. The only issue was when there's already the wrong alltemperate requirement (i.e. something changes based on singlepole specifically when alltemperate is enabled), in which case the requirement added by rscompat causes a contradiction.
Since this is already a nonsensical situation, I don't think it's worth trying to figure out which way to solve it (flipping the alltemperate requirement or dropping the singlepole requirement) ~> explicitly tell the user to fix it manually.
alienvalkyrie 
It is currently possible to have singlepole requirements without also checking that alltemperate is disabled. This is nonsensical, since when alltemperate is in fact enabled, singlepole should not make any difference.
I can see a number of ways to deal with this:
To make #44038 easier / possible at all, this needs to be resolved in S3_1.