setup: Make validate_config to check for specified archs #97
Conversation
klpbuild/klplib/codestream.py
Outdated
| for arch in archs: | ||
| if arch in self.archs and arch not in cs_config_archs: | ||
| raise RuntimeError(f"{self.name()}: {conf} not set on {arch}. Aborting") | ||
|
|
There was a problem hiding this comment.
Hi Marcos, the patch looks good to me.
however, a slight improvement could be this:
arch_config = self.get_all_configs(conf)
for arch in archs:
try:
conf_entry = arch_config.pop(arch)
# Here the body of the first for loop
except KeyError:
raise RuntimeError(f"{self.name()}: {conf} not set on {arch}. Aborting")This relies on pop() raising a KeyError when the arch is not present in the dictionary, at the same time might be slightly more maintainable and fast (less loops, and we avoid checking unaffected archs in the first loop).
But this is just me proposing useless improvements, so you can also discard this comment and merge as it is 😆
There was a problem hiding this comment.
Much better, indeed! What about the new version?
marcosps
force-pushed
the
arch-config-not-set
branch
from
805df16 to
b0fab54
Compare
klpbuild/klplib/codestream.py
Outdated
| try: | ||
| conf_entry = cs_config.pop(arch) | ||
| except KeyError as exc: | ||
| if arch not in self.archs: |
There was a problem hiding this comment.
I forgot we needed to check the arch is supported for the codestream. right!
Couldn't we just skip the iteration if the arch is not in self.archs? Like moving this if .. : continue right at the beginning of the loop, before the try?
Also, perhaps it could be a good idea to rename self.archs to self.supported_archs because I tend to make confusion with the archs supplied via arguments 😆
There was a problem hiding this comment.
Makes sense, I moved the if condition to the loop.
About changing self.archs to self.supported_archs is a bigger refactor, but maybe we can do in another moment :) Is that ok for now?
There was a problem hiding this comment.
Yeah sure, I meant renaming in another PR, I should have specified! My bad 😆
Up until now we could have a CONFIG_ being set on an archtecture and not on others and then it would pass the checks, only complaining for a missing symbol later on the setup phase. IIRC this used to work in the past, but somehow it got broken. Added a test to ensure that we catch such cases in the future. Signed-off-by: Marcos Paulo de Souza <[email protected]>
marcosps
force-pushed
the
arch-config-not-set
branch
from
b0fab54 to
18f83d5
Compare
|
lgtm, thanks! 🚀 |
| try: | ||
| conf_entry = cs_config.pop(arch) | ||
| except KeyError as exc: | ||
| raise RuntimeError(f"{self.name()}: {conf} not set on {arch}. Aborting") from exc |
There was a problem hiding this comment.
humm is it really necessary to abort here? Perhaps it would be better to just skip the codestream+arch combination.
There was a problem hiding this comment.
I thought about it, but it might require additional work on the TemplateGen, or maybe in how are handle it. Maybe we could just remove the arch from self.archs? I'm not sure, but yeah, sounds a better idea.
Up until now we could have a CONFIG_ being set on an archtecture and not on others and then it would pass the checks, only complaining for a missing symbol later on the setup phase.
IIRC this used to work in the past, but somehow it got broken. Added a test to ensure that we catch such cases in the future.