-
Notifications
You must be signed in to change notification settings - Fork 13
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
Support crypto config usage in depends.py
#47
Support crypto config usage in depends.py
#47
Conversation
Move the backup functionality from `depends.py` Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
f876d3f
to
436b56d
Compare
Add a new exception `CompileError` to differtiate that the exception raised during the compilation. Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
436b56d
to
20b9357
Compare
If the backup file already exists, it is presumed to be the desired backup, | ||
so don't make another backup. | ||
""" | ||
if self._backupname: |
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.
We are using instance attributes to control the execution flow of a metaclass? While it may work now, all it wold take to start doing unpredictable thigns is someone to set a default value anywhere in the inheritable chain.
CombinedConfig._backupname ----
| ----> MbedTLSConfigFile._backupname ----> ConfigFile._backupname
|
L ----> CryptoConfigFilee._backupname -----> ConfigFile._._backupname
Can we attack the logic to the actual file being present on the directory instead?
P.S: Why is ConfigFile a metaclass?
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.
We are using instance attributes to control the execution flow of a metaclass? While it may work now, all it wold take to start doing unpredictable thigns is someone to set a default value anywhere in the inheritable chain.
_backupname
is a simple private variable. Each instance has its own and it is not shared. Do not cause any problem.
Can we attack the logic to the actual file being present on the directory instead?
If a file with the desired file name is present then it will not create a copy. That will be the backup. This logic will preserve the original if more then one process is modifying the config.
P.S: Why is ConfigFile a metaclass?
It is abstract to force to create different derived classes for the different kind of config files to easily differentiate them.
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.
_backupname is a simple private variable.
Unfortunately that is not the case. What we care about is the lifecyle of the variable and weather we are creating it on runtime, before instantiation or during class constcution, at startup.
In the ConfigFile
class ConfigFile(metaclass=ABCMeta):
"""Representation of a configuration file."""
...
self._backupname = None
self._own_backup = False
Because that is a metaclass, that ConfigFile._backupname
is not just a class variable which is created after the __new__
method is called, but it is created as an attribute to __type__
and passed to the `__new__¬ consturctor to make the class. All classes that inherit it will have it as a class variable.
At any rate that code will work, but is very prone to shoot us in the foot since we do not contorl how the interpret will dynamically generate the classes. We can either add a comment to never change the default line, or only create _hasbackup
when the backup() method is called which is guaranteed to happen during runtime and after instantation. We can use hasattr() method to check if that is present.
It is abstract to force to create different derived classes for the different kind of config files to easily differentiate them.
You mean that we need them to be dynamically typed and not created using instantiation? Wouldn't simple inheritace work, with the cost of having to call init, when we actually have a config file?
Either way I won't block this pr for it, but I would like to point out that we are introducing complexity that brings more risk and technical debt than it's benefits.
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.
Because that is a metaclass, that
ConfigFile._backupname
is not just a class variable which is created after the__new__
method is called, but it is created as an attribute to__type__
and passed to the `__new__¬ consturctor to make the class. All classes that inherit it will have it as a class variable.
I think you are confused with the number of underscores. _
(one underscore) means it is a private variable (only naming convention, does nothing), __
(two underscores) uses name mangling and does what you mentioned.
Can be checked here.
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.
Thus regarding class ConfigFile(metaclass=ABCMeta):
after some offline discussion with @gabor-mezei-arm my understanding is that the intent is to declare the ConfigFile
as an abstract class to prevent it for being instantiated.
Looking at some resources on the web like https://docs.python.org/3/library/abc.html this seems a reasonable way to do it.
Otherwise because ConfigFile
is declared as class ConfigFile(metaclass=ABCMeta):
does not seem to mean that ConfigFile
is a metaclass. It seems to just mean that the object created to hold the definition of the ConfigFile
class is of type ABCMeta
and not type
. Then ConfigFile
class itself could be a metaclass but not necessarily and it does not seem to be with the signature of its init function as we have in config_common.py.
If I run the following script
#!/usr/bin/env python3
from abc import ABCMeta
class ConfigFile0:
def __init__(self):
self._backupname = None
class ConfigFile1(metaclass=ABCMeta):
def __init__(self):
self._backupname = None
print(type(ConfigFile0))
print(type(ConfigFile1))
class ConfigFile2(metaclass=ConfigFile1):
pass
I get:
<class 'type'>
<class 'abc.ABCMeta'>
Traceback (most recent call last):
File "/home/roncro01/./test1.py", line 16, in <module>
class ConfigFile2(metaclass=ConfigFile1):
TypeError: ConfigFile1.__init__() takes 1 positional argument but 4 were given
The init function of ConfigFile1
does not have the proper signature for a metaclass. It seems it should be something like: def __init__(cls, name, bases, dct):
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.
Just a type, otherwise, this looks good to me.
@@ -11,6 +11,15 @@ | |||
import sys | |||
import tempfile | |||
|
|||
class CompileError(Exception): | |||
"""Excetion to represent an error during the compilation.""" |
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.
"""Excetion to represent an error during the compilation.""" | |
"""Exception to represent an error during the compilation.""" |
If the backup file already exists, it is presumed to be the desired backup, | ||
so don't make another backup. | ||
""" | ||
if self._backupname: |
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.
Thus regarding class ConfigFile(metaclass=ABCMeta):
after some offline discussion with @gabor-mezei-arm my understanding is that the intent is to declare the ConfigFile
as an abstract class to prevent it for being instantiated.
Looking at some resources on the web like https://docs.python.org/3/library/abc.html this seems a reasonable way to do it.
Otherwise because ConfigFile
is declared as class ConfigFile(metaclass=ABCMeta):
does not seem to mean that ConfigFile
is a metaclass. It seems to just mean that the object created to hold the definition of the ConfigFile
class is of type ABCMeta
and not type
. Then ConfigFile
class itself could be a metaclass but not necessarily and it does not seem to be with the signature of its init function as we have in config_common.py.
If I run the following script
#!/usr/bin/env python3
from abc import ABCMeta
class ConfigFile0:
def __init__(self):
self._backupname = None
class ConfigFile1(metaclass=ABCMeta):
def __init__(self):
self._backupname = None
print(type(ConfigFile0))
print(type(ConfigFile1))
class ConfigFile2(metaclass=ConfigFile1):
pass
I get:
<class 'type'>
<class 'abc.ABCMeta'>
Traceback (most recent call last):
File "/home/roncro01/./test1.py", line 16, in <module>
class ConfigFile2(metaclass=ConfigFile1):
TypeError: ConfigFile1.__init__() takes 1 positional argument but 4 were given
The init function of ConfigFile1
does not have the proper signature for a metaclass. It seems it should be something like: def __init__(cls, name, bases, dct):
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.
LGTM
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.
Looks good
@gabor-mezei-arm we need a 3.6 PR that just update the framework pointer to the head of this PR to validate it for 3.6. Then we can merge it. |
Validated by #9292 (dev) and #9709 (3.6) CI. |
To support the
depnds.py
update:config_common.py
Related to Mbed-TLS/mbedtls#9292