Ticket #44181

Forbid singlepole setting requirements unless alltemperate is disabled

Open Date: 2022-03-25 22:54 Last Update: 2022-03-29 20:00

Reporter:
Owner:
Type:
Status:
Closed
Component:
MileStone:
Priority:
5 - Medium
Severity:
5 - Medium
Resolution:
Fixed
File:
4

Details

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
  • Mark all singlepole requirements deprecated
  • As above, but make it a hard ruleset loading failure

To make #44038 easier / possible at all, this needs to be resolved in S3_1.

Ticket History (3/17 Histories)

2022-03-25 22:54 Updated by: alienvalkyrie
  • New Ticket "Forbid singlepole setting requirements unless alltemperate is disabled" created
2022-03-25 23:09 Updated by: cazfi
Comment

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

I think we can simplify this be leaving (b) out, i.e., to require explicit !present alltemperate requirement even when the setting is locked to FALSE.

* As above, but make it a hard ruleset loading failure

We should do that for master (always), and for S3_1 outside compat mode. S3_1 compat mode, loading S3_0 ruleset, should make sensible conversion, and if we can't guarantee that the conversion is always correct, use compat->log_cb() to inform the user about the situation / instruct manual update.
2022-03-26 23:51 Updated by: alienvalkyrie
Comment

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.)

2022-03-27 02:50 Updated by: alienvalkyrie
  • Owner Update from (None) to alienvalkyrie
  • Component Update from General to Server
Comment

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?

2022-03-27 03:04 Updated by: alienvalkyrie
Comment

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

2022-03-27 03:12 Updated by: cazfi
Comment

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.

2022-03-27 03:14 Updated by: cazfi
Comment

Reply To alienvalkyrie

we might have to backport #43708 to S3_1

I'm not against.

2022-03-27 05:59 Updated by: alienvalkyrie
Comment

Reply To cazfi

Reply To alienvalkyrie

we might have to backport #43708 to S3_1

I'm not against.

Done #44203

2022-03-27 07:55 Updated by: alienvalkyrie
Comment

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)

2022-03-27 20:46 Updated by: alienvalkyrie
Comment

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.

2022-03-27 20:56 Updated by: cazfi
Comment

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.

2022-03-27 22:51 Updated by: alienvalkyrie
  • Resolution Update from None to Accepted
Comment

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.

2022-03-29 20:00 Updated by: alienvalkyrie
  • Status Update from Open to Closed
  • Resolution Update from Accepted to Fixed

Edit

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Login