-
Notifications
You must be signed in to change notification settings - Fork 59
sof-tplgreader: don't use RE for pipeline filtering #713
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
Conversation
If we have two pipelines with id=1 and id=10, current RE is not able to filter these two pipelines from each other. This patch deprecates RE and use string comparison for pipeline filtering. Signed-off-by: Chao Song <chao.song@linux.intel.com>
|
SOFCI TEST |
|
Timeout of APL_NOCODEC is gone with retest. I also tested this PR locally on UP2-nocodec and UpExtreme-nocodec, no issues reported. No idea what the pylint error means or if there's a better way to write this in Python but the concept is sound. Thanks @aiChaoSONG for the fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to "reverse-engineer" way too much code to review this small fix :-( #714 is a pure doc fix to avoid anyone having to go through the same pain again. Among other things it warns about the missing boolean precedence.
if type(value) == list:
The previous code supported only type(value) == list so please don't introduce this new "feature". Certainly not silently when fixing something unrelated and... probably never.
# check if current pipeline is the one we want
check if current pipeline is AMONG the ones we want
Modified version of thesofproject#713 for pure testing purposes. If we have two pipelines with id=1 and id=10, current RE is not able to filter these two pipelines from each other. This patch deprecates RE and use string comparison for pipeline filtering. Signed-off-by: Chao Song <chao.song@linux.intel.com>
If we have two pipelines with id=1 and id=10, current RE is not able to filter these two pipelines from each other. This patch deprecates RE and use string comparison for pipeline filtering. v2 by Marc: don't add support for non-lists, see thesofproject#713 review for details. Signed-off-by: Chao Song <chao.song@linux.intel.com>
If we have two pipelines with id=1 and id=10, current RE is not able to filter these two pipelines from each other. This patch deprecates RE and use string comparison for pipeline filtering. v2 by Marc: don't add support for non-lists, see thesofproject#713 review for details. Signed-off-by: Chao Song <chao.song@linux.intel.com> Signed-off-by: Marc Herbert <marc.herbert@intel.com>
| check = True if key in line.keys() else False | ||
| else: | ||
| # match for 'keyword'/'keyword [0-9]' target line | ||
| check = len ([em for em in value if re.match(em + '$|' + em + '[^a-zA-Z]', str(line[key]), re.I)]) > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of Pylint warnings are making sense.
tools/sof-tplgreader.py:3:0: W0611: Unused import subprocess (unused-import)
tools/sof-tplgreader.py:4:0: W0611: Unused import os (unused-import)
tools/sof-tplgreader.py:5:0: W0611: Unused import re (unused-import)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pylint warnings fixed in no functional change PR #714, please help review
marc-hb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code supported only type(value) == list so please don't introduce this new "feature". Certainly not silently when fixing something unrelated and... probably never.
@plbossart told me he needs this fix urgently so I applied my own suggestion above and resubmitted #715 to supersede this. Apologies.
| check = True if key in line.keys() else False | ||
| else: | ||
| # match for 'keyword'/'keyword [0-9]' target line | ||
| check = len ([em for em in value if re.match(em + '$|' + em + '[^a-zA-Z]', str(line[key]), re.I)]) > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pylint warnings fixed in no functional change PR #714, please help review
If we have two pipelines with id=1 and id=10, current RE is not able to filter these two pipelines from each other. This patch deprecates RE and use string comparison for pipeline filtering. v2 by Marc: don't add support for non-lists, see #713 review for details. Signed-off-by: Chao Song <chao.song@linux.intel.com> Signed-off-by: Marc Herbert <marc.herbert@intel.com>
|
@marc-hb it is correct to exclude the non-list support, actually I spend some time to understand |
|
First, I'm sorry I rushed #715 before we could have this discussion, that's bad. I blame @plbossart, he made me do it! He's always pushing everyone to rush things out... :'-D
Now back to the main question. This function is invoked in only one other function: I did spent a little bit of time looking at the removed regex code and I don't understand how it would make sense if value was not a list but only a string. In that case |
If we have two pipelines with id=1 and id=10, current
RE is not able to filter these two pipelines from each
other.
This patch deprecates RE and use string comparison for
pipeline filtering.
Signed-off-by: Chao Song chao.song@linux.intel.com