Skip to content
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

Scheduler: allow negation of regular expressions in plan class XML specs #4390

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

davidpanderson
Copy link
Contributor

Plan class XML specs allow regular expressions for OS, CPU vendor,
CPU model, project prefs, and host summary.
If any of these starts with a '!', negate the match.

This generalizes #4069.

Plan class XML specs allow regular expressions for OS, CPU vendor,
CPU model, project prefs, and host summary.
If any of these starts with a '!', negate the match.
@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #4390 (6c06524) into master (096cc15) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##             master   #4390      +/-   ##
===========================================
- Coverage      7.89%   7.89%   -0.01%     
  Complexity      793     793              
===========================================
  Files           277     277              
  Lines         35270   35276       +6     
  Branches       1180    1180              
===========================================
  Hits           2785    2785              
- Misses        32380   32386       +6     
  Partials        105     105              
Impacted Files Coverage Δ
sched/plan_class_spec.cpp 0.00% <0.00%> (ø)
sched/plan_class_spec.h 0.00% <0.00%> (ø)

@bema-aei
Copy link
Contributor

bema-aei commented Jun 2, 2021

Thanks! It's a good idea to centralize this in an own data type.

However in debugging I occasionally found it pretty hard to spot the reason for a missed match when I have to dig through a large plan class spec file for the right regex. Would it be feasible to keep the original regex string as well and print it in the debug message?

@Rytiss
Copy link
Contributor

Rytiss commented Jun 2, 2021

To negate a regex, you should use "negative lookaheads" (http://www.regular-expressions.info/lookaround.html, in particular "Regex Engine Internals" section); the way you implemented it isn't standard regex, e.g. !foo is not valid negative regex, but (?!foo) is. In fact, in valid regex !foo would be interpreted to look for an exclamation mark followed by foo.

The complication is that the negative match can be used anywhere in the regex, so a naive implementation cannot be used here - unless you deliberately choose to break regex compatibility.

@davidpanderson
Copy link
Contributor Author

The initial ! is not standard regex.

@Rytiss
Copy link
Contributor

Rytiss commented Jun 3, 2021

That's my point; shouldn't standard regex be supported?

@davidpanderson
Copy link
Contributor Author

? standard regex is supported. And also this notation for negation.

@Rytiss
Copy link
Contributor

Rytiss commented Jun 3, 2021

You are breaking compatibility with standard regex by introducing a special case for the first character being an exclamation mark. You can no longer use patterns that start with an exclamation mark. Since regexes do not necessarily start matching from the beginning of the string (e.g. ^ is not used), this could be a legitimate use case.

@davidpanderson
Copy link
Contributor Author

Bernd requested this feature, with this syntax. I think it's fine.

@bema-aei
Copy link
Contributor

bema-aei commented Jun 3, 2021

Actually Rytis requested "negative regex" in #4069. Einstein@Home also had a need for it (long time ago). At that time the POSIX regular expression library that is used here didn't support negation, or at least I wasn't aware of it. So I implemented this in a way that I found reasonably easy to remember and to implement, and offered that implementation in #4069 as an alternative.

If, however, the the POSIX regular expression library does support negation (now), I don't think we need this feature implemented separately at all. Did I miss something (now)?

@davidpanderson
Copy link
Contributor Author

AFAIK the POSIX interface doesn't support general negation.

@bema-aei
Copy link
Contributor

bema-aei commented Jun 3, 2021

  1. This proposal is non-standard, but if the current POSIX library doesn't offer the standard way, the only alternative I see would be to re-implement and augment it. I think this would require too much effort for too little benefit. However, maybe we should use a different library that supports negation?

  2. Is there a single character that would be better suited for this purpose than !, i.e. would not break the standard? Maybe two (e.g. (?!) would be ok?

  3. @Rytiss In your use case when matching a ! in a string not at the beginning, .! should still work.

@Rytiss
Copy link
Contributor

Rytiss commented Jun 3, 2021

The closest to the spec would be using (?! in the beginning and ) in the end (e.g. (?!foo)). This would not handle all negation cases (e.g. if you want to negate in the middle of the string), but would most likely be enough for our purposes, and would be future-compatible if we replace the regex library.

@bema-aei
Copy link
Contributor

bema-aei commented Jul 2, 2021

This might be a bit more effort to implement (checking for the closing parenthesis at the end and removing it for the regex comp), but should be worth it if it's really standard. Factoring out the regex handling in an own data type is a good idea anyway, and that way it has only to be done once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants