Repository: conan-io/conan
Pull request: add unknown package-ID in build-order
Changelog: Bugfix: Include "Package ID Unknown" nodes in ``conan graph build-order``, as they need to be processed in that order.
Docs: Omit

Close #6232

Problem statement:
[bug] Wrong conan graph build-order in package_revision_mode
### Environment Details
  * Operating System: Red Hat Enterprise Linux 7.4
  * Compiler: GCC 4.8.5
  * Conan version: 1.21.0
  * Python version: 3.7.1

### Steps to reproduce
Assumptions:
- Cache is clean; no binary packages exist so the expectation is a full build on the CI server will be necessary.
- libA depends on libC
- libB has no dependencies
- libC has no dependencies
- appX depends on libA and libB

Steps:
1. conan export libA ...
2. conan export libB ...
3. conan export libC ...
4. conan export appX ...
5. conan graph lock appX --lockfile appX.lock
6. conan build-order --build missing appX.lock

Package libA shows up as Unknown
Package libB shows up as Missing
Package libC shows up as Missing
Package appX shows up as Unknown

The problem is that the conan graph build-order output excludes libA since it is "Unknown". I expect that of appX, I suppose, since it's the application whose dependencies I'm trying to determine need building - but not of libA.

Thanks

Hints from issue discussion:
Hi @radonish 

Sorry for the delay in responding this.
I have been having a look at this, first a quick tip: at the moment you should also use ``conan graph lock appX --build=missing`` in the command ``graph lock`` too. Otherwise the lock might not work as expected.

Then, the behavior is correct. It is doing exactly what it is expected to do. "Unknown ID" nodes are not in the list of "to build" nodes because we don't know yet if they need to be built or not. It is possible that they don't. With ``package_revision_mode``, the dependencies resolution is exact, if after evaluating the package ID it is found as binary, then really no need to re-build it.

The way the package_revision_mode and build-order is intended to work is incremental, which by the way can be also more resource efficient:

- Compute the build-order
- Take the first level, fire those in parallel
- The moment any of those parallel jobs finish and some computing resources are available, compute the updated lock and **recompute** the build-order. This recomputation will also compute the final package ID that will be used instead of the Unknown one.
- Take that build order and repeat, making sure not relaunching jobs that are already running.

In this way, the utilization of resources is more effective. Imagine that the first level has 10 packages, 9 header-only libraries and one very heavy to build static library. Until the 10 finish, more builds cannot be launch, so 9 jobs will be not used until the static library finish. Packages depending on the header-only libraries could start as soon as those finish.

Said that, I think that it might be possible to add those to the output of build-order (without the package ID). I'll give it a try.

Commit message: add unknown in build-order

Patch to review:
diff --git a/conans/client/graph/graph.py b/conans/client/graph/graph.py
index ce1da041a31..92593502ca9 100644
--- a/conans/client/graph/graph.py
+++ b/conans/client/graph/graph.py
@@ -276,7 +276,7 @@ def new_build_order(self):
         for level in reversed(levels):
             new_level = []
             for n in level:
-                if n.binary == BINARY_BUILD and n.pref not in total_prefs:
+                if n.binary in (BINARY_UNKNOWN, BINARY_BUILD) and n.pref not in total_prefs:
                     new_level.append((n.id, n.pref.copy_clear_prev()))
                     total_prefs.add(n.pref)
             if new_level:
diff --git a/conans/test/functional/graph_lock/graph_lock_test.py b/conans/test/functional/graph_lock/graph_lock_test.py
index 69db873fb81..03dce1b66c4 100644
--- a/conans/test/functional/graph_lock/graph_lock_test.py
+++ b/conans/test/functional/graph_lock/graph_lock_test.py
@@ -619,6 +619,39 @@ def consumer_build_order_test(self):
         client.run("graph build-order conan.lock --build=missing")
         self.assertIn("test4/0.1", client.out)
 
+    def package_revision_mode_build_order_test(self):
+        # https://github.com/conan-io/conan/issues/6232
+        client = TestClient()
+        client.run("config set general.default_package_id_mode=package_revision_mode")
+        client.save({"conanfile.py": GenConanfile()})
+        client.run("export . libb/0.1@")
+        client.run("export . libc/0.1@")
+        client.save({"conanfile.py": GenConanfile().with_require_plain("libc/0.1")})
+        client.run("export . liba/0.1@")
+        client.save({"conanfile.py": GenConanfile().with_require_plain("liba/0.1")
+                                                   .with_require_plain("libb/0.1")})
+        client.run("export . app/0.1@")
+
+        client.run("graph lock app/0.1@ --build=missing")
+        client.run("graph build-order . --build=missing --json=bo.json")
+        self.assertIn("app/0.1:Package_ID_unknown - Unknown", client.out)
+        self.assertIn("liba/0.1:Package_ID_unknown - Unknown", client.out)
+        self.assertIn("libb/0.1:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9 - Build", client.out)
+        self.assertIn("libc/0.1:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9 - Build", client.out)
+        bo = client.load("bo.json")
+        bo = json.loads(bo)
+        libc_level = bo[0]
+        self.assertEqual("3", libc_level[0][0])
+        self.assertIn("libc/0.1", libc_level[0][1])
+        liba_level = bo[1]
+        self.assertEqual("2", liba_level[0][0])
+        self.assertIn("liba/0.1", liba_level[0][1])
+        self.assertEqual("4", liba_level[1][0])
+        self.assertIn("libb/0.1", liba_level[1][1])
+        libc_level = bo[2]
+        self.assertEqual("1", libc_level[0][0])
+        self.assertIn("app/0.1", libc_level[0][1])
+
 
 class GraphLockWarningsTestCase(unittest.TestCase):
 


Code review comments:
diff_hunk: @@ -619,6 +619,39 @@ def consumer_build_order_test(self):
         client.run("graph build-order conan.lock --build=missing")
         self.assertIn("test4/0.1", client.out)
 
+    def package_revision_mode_build_order_test(self):
+        # https://github.com/conan-io/conan/issues/6232
+        client = TestClient()
+        client.run("config set general.default_package_id_mode=package_revision_mode")
+        client.save({"conanfile.py": GenConanfile()})
+        client.run("export . libb/0.1@")
+        client.run("export . libc/0.1@")
+        client.save({"conanfile.py": GenConanfile().with_require_plain("libc/0.1")})
+        client.run("export . liba/0.1@")
+        client.save({"conanfile.py": GenConanfile().with_require_plain("liba/0.1")
+                                                   .with_require_plain("libb/0.1")})
+        client.run("export . app/0.1@")
+
+        client.run("graph lock app/0.1@ --build=missing")
+        client.run("graph build-order . --build=missing --json=bo.json")
+        self.assertIn("app/0.1:Package_ID_unknown - Unknown", client.out)
+        self.assertIn("liba/0.1:Package_ID_unknown - Unknown", client.out)
+        self.assertIn("libb/0.1:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9 - Build", client.out)
+        self.assertIn("libc/0.1:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9 - Build", client.out)
+        bo = client.load("bo.json")
+        bo = json.loads(bo)
path: conans/test/functional/graph_lock/graph_lock_test.py
text: @user1:
IMO it would we easier to read if we compare the full JSON, otherwise, the following asserts are quite hard to understand.

Something like:

```
self.assertEqual(sorted(bo.items()), sorted(json.loads("""
[
   [
      ["3","libc/0.1#f3367e0e7d170aa12abccb175fee5f97:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9"]
   ], 
   [
      ["2","liba/0.1#7086607aa6efbad8e2527748e3ee8237:Package_ID_unknown"],
      ["4","libb/0.1#f3367e0e7d170aa12abccb175fee5f97:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9"]
   ],
   [
      ["1","app/0.1#7742ee9e2f19af4f9ed7619f231ca871:Package_ID_unknown"]
   ]
]
""")))
```