Merge pull request #7237 from p12tic/buildstep-prevent-setattr

process: Warn when changing buildstep attributes after it is created

common/code_spelling_ignore_words.txt

@@ -437,6 +437,7 @@ downloadstring
 dradez
 dss
 du
+dunder
 dup
 durations
 durchmesser

master/buildbot/buildbot_net_usage_data.py

@@ -89,7 +89,7 @@ def getName(obj):
     def sanitize(name):
         return name.replace(".", "/")
     if isinstance(obj, _BuildStepFactory):
-        klass = obj.factory
+        klass = obj.step_class
     else:
         klass = type(obj)
     name = ""

master/buildbot/process/build.py

@@ -330,7 +330,9 @@ def startBuild(self, workerforbuilder):
         # the preparation step counts the time needed for preparing the worker and getting the
         # locks.
         # we cannot use a real step as we don't have a worker yet.
-        self.preparation_step = buildstep.BuildStep(name="worker_preparation")
+        self.preparation_step = buildstep.create_step_from_step_or_factory(
+            buildstep.BuildStep(name="worker_preparation")
+        )
         self.preparation_step.setBuild(self)
         yield self.preparation_step.addStep()
 

master/buildbot/process/buildstep.py

@@ -133,16 +133,19 @@ class _BuildStepFactory(util.ComparableMixin):
     """
     compare_attrs = ('factory', 'args', 'kwargs')
 
-    def __init__(self, factory, *args, **kwargs):
-        self.factory = factory
+    def __init__(self, step_class, *args, **kwargs):
+        self.step_class = step_class
         self.args = args
         self.kwargs = kwargs
 
     def buildStep(self):
         try:
-            return self.factory(*self.args, **self.kwargs)
+            step = object.__new__(self.step_class)
+            step._factory = self
+            step.__init__(*self.args, **self.kwargs)  # noqa pylint: disable=unnecessary-dunder-call
+            return step
         except Exception:
-            log.msg(f"error while creating step, factory={self.factory}, args={self.args}, "
+            log.msg(f"error while creating step, step_class={self.step_class}, args={self.args}, "
                     f"kwargs={self.kwargs}")
             raise
 
@@ -165,6 +168,43 @@ def create_step_from_step_or_factory(step_or_factory):
     return get_factory_from_step_or_factory(step_or_factory).buildStep()
 
 
+class BuildStepWrapperMixin:
+    __init_completed = False
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.__init_completed = True
+
+    def __setattr__(self, name, value):
+        if self.__init_completed:
+            warn_deprecated(
+                "3.10.0",
+                "Changes to attributes of a BuildStep instance are ignored, this is a bug. "
+                "Use set_step_arg(name, value) for that."
+            )
+        super().__setattr__(name, value)
+
+
+# This is also needed for comparisons to work because ComparableMixin requires type(x) and
+# x.__class__ to be equal in order to perform comparison at all.
+_buildstep_wrapper_cache = {}
+
+
+def _create_buildstep_wrapper_class(klass):
+    class_id = id(klass)
+    cached = _buildstep_wrapper_cache.get(class_id, None)
+    if cached is not None:
+        return cached
+
+    wrapper = type(
+        klass.__qualname__,
+        (BuildStepWrapperMixin, klass),
+        {}
+    )
+    _buildstep_wrapper_cache[class_id] = wrapper
+    return wrapper
+
+
 @implementer(interfaces.IBuildStep)
 class BuildStep(results.ResultComputingConfigMixin,
                 properties.PropertiesMixin,
@@ -287,13 +327,34 @@ def __init__(self, **kwargs):
         self.results = None
         self._start_unhandled_deferreds = None
         self._interrupt_deferwaiter = deferwaiter.DeferWaiter()
+        self._update_summary_debouncer = debounce.Debouncer(
+            1.0,
+            self._update_summary_impl,
+            lambda: self.master.reactor
+        )
         self._test_result_submitters = {}
 
     def __new__(klass, *args, **kwargs):
-        self = object.__new__(klass)
+        # The following code prevents changing BuildStep attributes after an instance
+        # is created during config time. Such attribute changes don't affect the factory,
+        # so they will be lost when actual build step is created.
+        #
+        # This is implemented by dynamically creating a subclass that disallows attribute
+        # writes after __init__ completes.
+        self = object.__new__(_create_buildstep_wrapper_class(klass))
         self._factory = _BuildStepFactory(klass, *args, **kwargs)
         return self
 
+    def is_exact_step_class(self, klass):
+        # Due to wrapping BuildStep in __new__, it's not possible to compare self.__class__ to
+        # check if self is an instance of some class (but not subclass).
+        if self.__class__ is klass:
+            return True
+        mro = self.__class__.mro()
+        if len(mro) >= 3 and mro[1] is BuildStepWrapperMixin and mro[2] is klass:
+            return True
+        return False
+
     def __str__(self):
         args = [repr(x) for x in self._factory.args]
         args.extend([str(k) + "=" + repr(v)
@@ -347,6 +408,15 @@ def getProperties(self):
     def get_step_factory(self):
         return self._factory
 
+    def set_step_arg(self, name, value):
+        self._factory.kwargs[name] = value
+        # check if buildstep can still be constructed with the new arguments
+        try:
+            self._factory.buildStep()
+        except Exception:
+            log.msg(f"Cannot set step factory attribute {name} to {value}: step creation fails")
+            raise
+
     def setupProgress(self):
         # this function temporarily does nothing
         pass
@@ -387,9 +457,11 @@ def getBuildResultSummary(self):
             summary['build'] = summary['step']
         return summary
 
-    @debounce.method(wait=1)
-    @defer.inlineCallbacks
     def updateSummary(self):
+        self._update_summary_debouncer()
+
+    @defer.inlineCallbacks
+    def _update_summary_impl(self):
         def methodInfo(m):
             lines = inspect.getsourcelines(m)
             return "\nat {}:{}:\n {}".format(inspect.getsourcefile(m), lines[1],
@@ -513,7 +585,7 @@ def startStep(self, remote):
         # update the summary one last time, make sure that completes,
         # and then don't update it any more.
         self.updateSummary()
-        yield self.updateSummary.stop()
+        yield self._update_summary_debouncer.stop()
 
         for sub in self._test_result_submitters.values():
             yield sub.finish()

master/buildbot/steps/shell.py

@@ -158,8 +158,7 @@ class ShellCommand(buildstep.ShellMixin, buildstep.BuildStep):
     name = 'shell'
 
     def __init__(self, **kwargs):
-
-        if self.__class__ is ShellCommand:
+        if self.is_exact_step_class(ShellCommand):
             if 'command' not in kwargs:
                 config.error("ShellCommand's `command' argument is not specified")
 
@@ -263,7 +262,7 @@ def __init__(self,
             self.warningExtractor = WarningCountingShellCommand.warnExtractWholeLine
         self.maxWarnCount = maxWarnCount
 
-        if self.__class__ is WarningCountingShellCommand and not kwargs.get('command'):
+        if self.is_exact_step_class(WarningCountingShellCommand) and not kwargs.get('command'):
             # WarningCountingShellCommand class is directly instantiated.
             # Explicitly check that command is set to prevent runtime error
             # later.

master/buildbot/steps/vstudio.py

@@ -512,7 +512,7 @@ def run(self):
         if self.platform is None:
             config.error('platform is mandatory. Please specify a string such as "Win32"')
 
-        yield self.updateSummary()
+        self.updateSummary()
 
         command = (f'"%VCENV_BAT%" x86 && msbuild "{self.projectfile}" '
                    f'/p:Configuration="{self.config}" /p:Platform="{self.platform}" /maxcpucount')
@@ -572,7 +572,7 @@ def run(self):
 
         self.description = 'building ' + self.describe_project()
         self.descriptionDone = 'built ' + self.describe_project()
-        yield self.updateSummary()
+        self.updateSummary()
 
         command = ('FOR /F "tokens=*" %%I in '
                    f'(\'vswhere.exe -version "{self.version_range}" -products * '

master/buildbot/test/integration/test_worker.py

@@ -21,6 +21,7 @@
 from buildbot.interfaces import IBuildStepFactory
 from buildbot.machine.base import Machine
 from buildbot.process.buildstep import BuildStep
+from buildbot.process.buildstep import create_step_from_step_or_factory
 from buildbot.process.factory import BuildFactory
 from buildbot.process.properties import Interpolate
 from buildbot.process.results import CANCELLED
@@ -45,7 +46,8 @@ def __init__(self, **kwargs):
 
     def buildStep(self):
         step_deferred = defer.Deferred()
-        step = ControllableStep(step_deferred, **self.kwargs)
+        step = create_step_from_step_or_factory(
+            ControllableStep(step_deferred, **self.kwargs))
         self.steps.append((step, step_deferred))
         return step
 
@@ -213,7 +215,8 @@ def test_worker_reconfigure_with_new_builder(self):
                           workernames=['local1'],
                           factory=BuildFactory()),
         ]
-        config_dict['workers'] = [self.createLocalWorker('local1', max_builds=2)]
+        config_dict['workers'] = [
+            self.createLocalWorker('local1', max_builds=2)]
 
         # reconfig should succeed
         yield self.reconfig_master(config_dict)
@@ -230,7 +233,8 @@ def test_step_with_worker_build_props_during_worker_disconnect(self):
         config_dict = {
             'builders': [
                 BuilderConfig(name="builder", workernames=['local'],
-                              properties={'worker': Interpolate("%(worker:numcpus)s")},
+                              properties={'worker': Interpolate(
+                                  "%(worker:numcpus)s")},
                               factory=BuildFactory([stepcontroller.step])),
             ],
             'workers': [controller.worker],