making aggregate_components non-destructive, very dangerous (#10183)

* making aggregate_components non-destructive, very dangerous

* adding a red/green test and removing traceback printing

conan/tools/apple/xcodedeps.py

@@ -174,8 +174,7 @@ def _content(self):
         for dep in self._conanfile.dependencies.host.values():
             dep_name = dep.ref.name
             dep_name = dep_name.replace(".", "_").replace("-", "_")
-            cpp_info = dep.cpp_info.copy()
-            cpp_info.aggregate_components()
+            cpp_info = dep.cpp_info.aggregated_components()
             public_deps = [d.ref.name.replace(".", "_").replace("-", "_")
                            for r, d in dep.dependencies.direct_host.items() if r.visible]
 

conan/tools/cmake/cmakedeps/templates/target_data.py

@@ -3,7 +3,6 @@
 
 from conan.tools.cmake.cmakedeps.templates import CMakeDepsFileTemplate
 from conan.tools.cmake.utils import get_file_name
-from conans.errors import ConanException
 
 """
 
@@ -25,16 +24,17 @@ def filename(self):
 
     @property
     def context(self):
-        global_cpp = self.get_global_cpp_cmake()
+        global_cpp = self._get_global_cpp_cmake()
         if not self.build_modules_activated:
             global_cpp.build_modules_paths = ""
 
+        components = self._get_required_components_cpp()
         # using the target names to name components, may change in the future?
         components_names = " ".join([components_target_name for components_target_name, _ in
-                                    reversed(self.get_required_components_cpp())])
+                                    reversed(components)])
 
         components_cpp = [(cmake_target_name.replace("::", "_"), cmake_target_name, cpp)
-                          for cmake_target_name, cpp in self.get_required_components_cpp()]
+                          for cmake_target_name, cpp in components]
 
         # For the build requires, we don't care about the transitive (only runtime for the br)
         # so as the xxx-conf.cmake files won't be generated, don't include them as find_dependency
@@ -110,13 +110,12 @@ def template(self):
           """)
         return ret
 
-    def get_global_cpp_cmake(self):
-        global_cppinfo = self.conanfile.cpp_info.copy()
-        global_cppinfo.aggregate_components()
+    def _get_global_cpp_cmake(self):
+        global_cppinfo = self.conanfile.cpp_info.aggregated_components()
         pfolder_var_name = "{}_PACKAGE_FOLDER{}".format(self.pkg_name, self.config_suffix)
-        return DepsCppCmake(global_cppinfo, pfolder_var_name)
+        return _TargetDataContext(global_cppinfo, pfolder_var_name)
 
-    def get_required_components_cpp(self):
+    def _get_required_components_cpp(self):
         """Returns a list of (component_name, DepsCppCMake)"""
         ret = []
         sorted_comps = self.conanfile.cpp_info.get_sorted_components()
@@ -125,7 +124,7 @@ def get_required_components_cpp(self):
                                                                   "direct": True})
         for comp_name, comp in sorted_comps.items():
             pfolder_var_name = "{}_PACKAGE_FOLDER{}".format(self.pkg_name, self.config_suffix)
-            deps_cpp_cmake = DepsCppCmake(comp, pfolder_var_name)
+            deps_cpp_cmake = _TargetDataContext(comp, pfolder_var_name)
             public_comp_deps = []
             for require in comp.requires:
                 if "::" in require:  # Points to a component of a different package
@@ -157,7 +156,7 @@ def _get_dependency_filenames(self):
         return ret
 
 
-class DepsCppCmake(object):
+class _TargetDataContext(object):
 
     def __init__(self, cpp_info, pfolder_var_name):
 

conan/tools/cmake/toolchain.py

@@ -434,8 +434,7 @@ def context(self):
         # Read the buildirs
         build_paths = []
         for req in host_req:
-            cppinfo = req.cpp_info.copy()
-            cppinfo.aggregate_components()
+            cppinfo = req.cpp_info.aggregated_components()
             build_paths.extend([os.path.join(req.package_folder,
                                        p.replace('\\', '/').replace('$', '\\$').replace('"', '\\"'))
                                 for p in cppinfo.builddirs])

conan/tools/env/virtualrunenv.py

@@ -9,9 +9,9 @@ def runenv_from_cpp_info(conanfile, dep, os_name):
     # FIXME: Remove conanfile arg
     dyn_runenv = Environment()
 
-    cpp_info = dep.cpp_info
-    cpp_info.aggregate_components()
+    cpp_info = dep.cpp_info.aggregated_components()
     pkg_folder = dep.package_folder
+    # FIXME: This code is dead, cpp_info cannot be None
     if cpp_info is None:  # This happens when the dependency is a private one = BINARY_SKIP
         return dyn_runenv
 

conan/tools/gnu/autotoolsdeps.py

@@ -13,11 +13,10 @@ def __init__(self, conanfile):
     def _get_cpp_info(self):
         ret = NewCppInfo()
         for dep in self._conanfile.dependencies.host.values():
-            dep_cppinfo = dep.cpp_info.copy()
+            dep_cppinfo = dep.cpp_info.aggregated_components()
             dep_cppinfo.set_relative_base_folder(dep.package_folder)
             # In case we have components, aggregate them, we do not support isolated
             # "targets" with autotools
-            dep_cppinfo.aggregate_components()
             ret.merge(dep_cppinfo)
         return ret
 

conan/tools/google/bazeldeps.py

@@ -65,34 +65,34 @@ def _get_dependency_buildfile_content(self, dependency):
 
         """)
 
-        dependency.cpp_info.aggregate_components()
+        cpp_info = dependency.cpp_info.aggregated_components()
 
-        if not dependency.cpp_info.libs and not dependency.cpp_info.includedirs:
+        if not cpp_info.libs and not cpp_info.includedirs:
             return None
 
         headers = []
         includes = []
 
-        for path in dependency.cpp_info.includedirs:
+        for path in cpp_info.includedirs:
             headers.append('"{}/**"'.format(path))
             includes.append('"{}"'.format(path))
 
         headers = ', '.join(headers)
         includes = ', '.join(includes)
 
         defines = ('"{}"'.format(define.replace('"', "'"))
-                   for define in dependency.cpp_info.defines)
+                   for define in cpp_info.defines)
         defines = ', '.join(defines)
 
         linkopts = []
-        for linkopt in dependency.cpp_info.system_libs:
+        for linkopt in cpp_info.system_libs:
             linkopts.append('"-l{}"'.format(linkopt))
         linkopts = ', '.join(linkopts)
 
         context = {
             "name": dependency.ref.name,
-            "libs": dependency.cpp_info.libs,
-            "libdir": dependency.cpp_info.libdirs[0],
+            "libs": cpp_info.libs,
+            "libdir": cpp_info.libdirs[0],
             "headers": headers,
             "includes": includes,
             "defines": defines,

conan/tools/microsoft/msbuilddeps.py

@@ -283,8 +283,7 @@ def _content(self):
         for dep in host_req + test_req:
             dep_name = dep.ref.name
             dep_name = dep_name.replace(".", "_")
-            cpp_info = dep.cpp_info.copy()
-            cpp_info.aggregate_components()
+            cpp_info = dep.cpp_info.aggregated_components()
             public_deps = [d.ref.name.replace(".", "_")
                            for r, d in dep.dependencies.direct_host.items() if r.visible]
             # One file per configuration, with just the variables
@@ -302,8 +301,7 @@ def _content(self):
         for dep in build_req:
             dep_name = dep.ref.name
             dep_name = dep_name.replace(".", "_") + "_build"
-            cpp_info = dep.cpp_info.copy()
-            cpp_info.aggregate_components()
+            cpp_info = dep.cpp_info.aggregated_components()
             public_deps = [d.ref.name.replace(".", "_")
                            for r, d in dep.dependencies.direct_host.items() if r.visible]
             # One file per configuration, with just the variables

conans/model/new_build_info.py

@@ -8,6 +8,7 @@
                    "frameworkdirs"]
 _FIELD_VAR_NAMES = ["system_libs", "frameworks", "libs", "defines", "cflags", "cxxflags",
                     "sharedlinkflags", "exelinkflags", "objects"]
+_ALL_NAMES = _DIRS_VAR_NAMES + _FIELD_VAR_NAMES
 
 
 class _NewComponent(object):
@@ -78,6 +79,7 @@ def __init__(self):
         self.components = DefaultOrderedDict(lambda: _NewComponent())
         # Main package is a component with None key
         self.components[None] = _NewComponent()
+        self._aggregated = None  # A _NewComponent object with all the components aggregated
 
     def __getattr__(self, attr):
         return getattr(self.components[None], attr)
@@ -97,11 +99,14 @@ def component_names(self):
         return filter(None, self.components.keys())
 
     def merge(self, other):
-        """Merge 'other' into self. 'other' can be an old cpp_info object"""
+        """Merge 'other' into self. 'other' can be an old cpp_info object
+        Used to merge Layout source + build cpp objects info (editables)
+        :type other: NewCppInfo
+        """
         def merge_list(o, d):
             d.extend(e for e in o if e not in d)
 
-        for varname in _DIRS_VAR_NAMES + _FIELD_VAR_NAMES:
+        for varname in _ALL_NAMES:
             other_values = getattr(other, varname)
             if other_values is not None:
                 current_values = self.components[None].get_init(varname, [])
@@ -122,7 +127,7 @@ def merge_list(o, d):
         for cname, c in other.components.items():
             if cname is None:
                 continue
-            for varname in _DIRS_VAR_NAMES + _FIELD_VAR_NAMES:
+            for varname in _ALL_NAMES:
                 other_values = getattr(c, varname)
                 if other_values is not None:
                     current_values = self.components[cname].get_init(varname, [])
@@ -161,42 +166,36 @@ def get_sorted_components(self):
 
         return OrderedDict([(cname,  self.components[cname]) for cname in processed])
 
-    def aggregate_components(self):
-        """Aggregates all the components as global values"""
-
-        if self.has_components:
-            components = self.get_sorted_components()
-            cnames = list(components.keys())
-            cnames.reverse()  # More dependant first
-
-            # Clean global values
-            for n in _DIRS_VAR_NAMES + _FIELD_VAR_NAMES:
-                setattr(self.components[None], n, [])
-
-            for name in cnames:
-                component = components[name]
-                for n in _DIRS_VAR_NAMES + _FIELD_VAR_NAMES:
-                    if getattr(component, n):
-                        dest = getattr(self.components[None], n)
-                        if dest is None:
-                            setattr(self.components[None], n, [])
-                        dest += [i for i in getattr(component, n) if i not in dest]
-
-                # NOTE: The properties are not aggregated because they might refer only to the
-                # component like "cmake_target_name" describing the target name FOR THE component
-                # not the namespace.
-
-                if component.requires:
-                    current_values = self.components[None].get_init("requires", [])
-                    current_values.extend(component.requires)
-
-            # FIXME: What to do about sysroot?
-            # Leave only the aggregated value
-            main_value = self.components[None]
-            self.components = DefaultOrderedDict(lambda: _NewComponent())
-            self.components[None] = main_value
+    def aggregated_components(self):
+        """Aggregates all the components as global values, returning a new NewCppInfo"""
+        if self._aggregated is None:
+            if self.has_components:
+                result = _NewComponent()
+                for n in _ALL_NAMES:  # Initialize all values, from None => []
+                    setattr(result, n, [])  # TODO: This is a bit dirty
+                # Reversed to make more dependant first
+                for name, component in reversed(self.get_sorted_components().items()):
+                    for n in _ALL_NAMES:
+                        if getattr(component, n):
+                            dest = result.get_init(n, [])
+                            dest.extend([i for i in getattr(component, n) if i not in dest])
+
+                    # NOTE: The properties are not aggregated because they might refer only to the
+                    # component like "cmake_target_name" describing the target name FOR THE component
+                    # not the namespace.
+                    if component.requires:
+                        current_values = result.get_init("requires", [])
+                        current_values.extend(component.requires)
+
+                # FIXME: What to do about sysroot?
+            else:
+                result = copy.copy(self.components[None])
+            self._aggregated = NewCppInfo()
+            self._aggregated.components[None] = result
+        return self._aggregated
 
     def copy(self):
+        # Only used at the moment by layout() editable merging build+source .cpp data
         ret = NewCppInfo()
         ret._generator_properties = copy.copy(self._generator_properties)
         ret.components = DefaultOrderedDict(lambda: _NewComponent())
@@ -219,7 +218,7 @@ def clear_none(self):
         """A field with None meaning is 'not declared' but for consumers, that is irrelevant, an
         empty list is easier to handle and makes perfect sense."""
         for c in self.components.values():
-            for varname in _DIRS_VAR_NAMES + _FIELD_VAR_NAMES:
+            for varname in _ALL_NAMES:
                 if getattr(c, varname) is None:
                     setattr(c, varname, [])
             if c.requires is None:
@@ -232,7 +231,7 @@ def clear_none(self):
     def __str__(self):
         ret = []
         for cname, c in self.components.items():
-            for n in _DIRS_VAR_NAMES + _FIELD_VAR_NAMES:
+            for n in _ALL_NAMES:
                 ret.append("Component: '{}' "
                            "Var: '{}' "
                            "Value: '{}'".format(cname, n, getattr(c, n)))
@@ -280,7 +279,7 @@ def fill_old_cppinfo(origin, old_cpp):
         for cname, c in origin.components.items():
             if cname is None:
                 continue
-            for varname in _DIRS_VAR_NAMES + _FIELD_VAR_NAMES:
+            for varname in _ALL_NAMES:
                 value = getattr(c, varname)
                 if value is not None:
                     # Override the self.cpp_info component value
@@ -291,7 +290,7 @@ def fill_old_cppinfo(origin, old_cpp):
             if c._generator_properties is not None:
                 old_cpp.components[cname]._generator_properties = copy.copy(c._generator_properties)
     else:
-        for varname in _DIRS_VAR_NAMES + _FIELD_VAR_NAMES:
+        for varname in _ALL_NAMES:
             value = getattr(origin, varname)
             if value is not None:
                 # Override the self.cpp_info value

conans/test/integration/toolchains/cmake/cmakedeps/test_cmakedeps.py

@@ -114,3 +114,42 @@ def package_info(self):
         content = f.read()
         assert 'set(hello_OBJECTS_RELEASE "${hello_PACKAGE_FOLDER_RELEASE}/mycomponent.o")' in content
         assert 'set(hello_hello_say_OBJECTS_RELEASE "${hello_PACKAGE_FOLDER_RELEASE}/mycomponent.o")' in content
+
+
+def test_cpp_info_component_error_aggregate():
+    # https://github.com/conan-io/conan/issues/10176
+    # This test was consistently failing because "VirtualRunEnv" was not doing a "copy()"
+    # of cpp_info before calling "aggregate_components()", and it was destructive, removing
+    # components data
+    client = TestClient()
+    conan_hello = textwrap.dedent("""
+        from conan import ConanFile
+        class Pkg(ConanFile):
+            def package_info(self):
+                self.cpp_info.components["say"].includedirs = ["include"]
+            """)
+    consumer = textwrap.dedent("""
+        from conans import ConanFile
+        class Pkg(ConanFile):
+            settings = "os", "compiler", "arch", "build_type"
+            requires = "hello/1.0"
+            generators = "VirtualRunEnv", "CMakeDeps"
+            def package_info(self):
+                self.cpp_info.components["chat"].requires = ["hello::say"]
+        """)
+    test_package = textwrap.dedent("""
+        from conans import ConanFile
+        class Pkg(ConanFile):
+            settings = "os", "compiler", "arch", "build_type"
+            generators = "VirtualRunEnv", "CMakeDeps"
+
+            def test(self):
+                pass
+        """)
+
+    client.save({"hello/conanfile.py": conan_hello,
+                 "consumer/conanfile.py": consumer,
+                 "consumer/test_package/conanfile.py": test_package})
+    client.run("create hello hello/1.0@")
+    client.run("create consumer consumer/1.0@")
+    assert "consumer/1.0 (test package): Running test()" in client.out

conans/test/unittests/conan_build_info/new_build_info_test.py

@@ -62,9 +62,7 @@ def test_component_aggregation():
     cppinfo.components["c1"].set_property("my_foo", "jander")
     cppinfo.components["c1"].set_property("my_foo2", "bar2", "other_gen")
 
-
-    ret = cppinfo.copy()
-    ret.aggregate_components()
+    ret = cppinfo.aggregated_components()
 
     assert ret.includedirs == ["includedir_c1", "includedir_c2"]
     assert ret.libdirs == ["libdir_c1", "libdir_c2"]
@@ -85,8 +83,8 @@ def test_component_aggregation():
     cppinfo.components["c1"].requires = []
     cppinfo.components["c2"].requires = ["c1"]
 
-    ret = cppinfo.copy()
-    ret.aggregate_components()
+    cppinfo._aggregated = None  # Dirty, just to force recomputation
+    ret = cppinfo.aggregated_components()
 
     assert ret.includedirs == ["includedir_c2", "includedir_c1"]
     assert ret.libdirs == ["libdir_c2", "libdir_c1"]
@@ -130,8 +128,8 @@ def test_cpp_info_merge_aggregating_components_first(aggregate_first):
     other.components["boo"].requires = ["boo2"]  # Deterministic order
 
     if aggregate_first:
-        cppinfo.aggregate_components()
-        other.aggregate_components()
+        cppinfo = cppinfo.aggregated_components()
+        other = other.aggregated_components()
 
     cppinfo.merge(other)