Declare library dependency introduced by https://reviews.llvm.org/D59634

[labath]

Thanks. I was just looking at bot failure I got and about to submit a fix.

I don't believe this is the right fix though. The correct way to add dependencies should be through LLVMBuild.txt. I am going to submit that soon.

[jhenderson]

I can't see how D59482 introduces a dependency on LLVMObject in the main LLVMObjectYAML library. It does add unit tests that rely on the Object library, and includes the necessary reference. Where is the new dependency that this is needed for?

[labath]

In D60127#1451385, @jhenderson wrote:

I can't see how D59482 introduces a dependency on LLVMObject in the main LLVMObjectYAML library. It does add unit tests that rely on the Object library, and includes the necessary reference. Where is the new dependency that this is needed for?

I think serge has linked the wrong patch. The culprit is https://reviews.llvm.org/D59634, which does add the dependency because it uses the Object library to construct the ObjectYAML model. This is a new dependency because for all the other binary formats the code to construct the yaml model lives in tool code (obj2yaml). Here, I chose to do that in the ObjectYAML library because I wanted to make that code accessible to unit tests.

@jhenderson do you see any problems with this new dependency (i.e., should I push a patch adding it), or does it need more discussion (i.e., revert the original patch) ? (FWIW, to me it seems natural for ObjectYAML to depend on Object, and I don't think this creates any danger of dependency cycles, as Object should never depend on ObjectYAML.)

[jhenderson]

In D60127#1451390, @labath wrote:

In D60127#1451385, @jhenderson wrote:

I can't see how D59482 introduces a dependency on LLVMObject in the main LLVMObjectYAML library. It does add unit tests that rely on the Object library, and includes the necessary reference. Where is the new dependency that this is needed for?

I think serge has linked the wrong patch. The culprit is https://reviews.llvm.org/D59634, which does add the dependency because it uses the Object library to construct the ObjectYAML model. This is a new dependency because for all the other binary formats the code to construct the yaml model lives in tool code (obj2yaml). Here, I chose to do that in the ObjectYAML library because I wanted to make that code accessible to unit tests.

Ah, that makes more sense, thanks.

@jhenderson do you see any problems with this new dependency (i.e., should I push a patch adding it), or does it need more discussion (i.e., revert the original patch) ? (FWIW, to me it seems natural for ObjectYAML to depend on Object, and I don't think this creates any danger of dependency cycles, as Object should never depend on ObjectYAML.)

My main concern is the number of files needed to build obj2yaml/yaml2obj, which would dramatically increase with that dependency. That being said, most of those files would be needed to be built for the tools that these two executables are used to test, so I don't think it's a genuine issue, and what you say about naturalness does make sense, so please go ahead and fix it.

[labath]

In D60127#1451391, @jhenderson wrote:

My main concern is the number of files needed to build obj2yaml/yaml2obj, which would dramatically increase with that dependency. That being said, most of those files would be needed to be built for the tools that these two executables are used to test, so I don't think it's a genuine issue, and what you say about naturalness does make sense, so please go ahead and fix it.

Thanks. It looks like somebody already beat me to it (r357471).

I don't think this will have any affect on the dependency graph of these two tools, as they already have this dependency in their own CMakeLists.txt files. This makes total sense for obj2yaml, as it uses the object library to parse the object files. I'm not entirely sure why this is needed for yaml2obj, but the dependency seems to be coming from the wasm and coff code.

[serge-sans-paille]

Obsoleted by r357471

lib/ObjectYAML/CMakeLists.txt

   MinidumpYAML.cpp
   WasmYAML.cpp
   YAML.cpp
+
+  DEPENDS
+  LLVMObject
+
+  LINK_LIBS
+  LLVMObject
   )