Work around unexecuted member for installed libraries issue

See comment for the long-term plan.

4 files changed, 64 insertions, 21 deletions

diff --git a/libbuild2/algorithm.hxx b/libbuild2/algorithm.hxx
index c8f1d81..c6fb1c2 100644
--- a/libbuild2/algorithm.hxx
+++ b/libbuild2/algorithm.hxx
@@ -434,7 +434,8 @@ namespace build2
                          bool fail = true);
 
   // Apply the specified recipe directly and without incrementing the
-  // dependency counts. The target must be locked.
+  // dependency counts. The target must be locked (and it remains locked
+  // after this function returns).
   //
   // Note that there will be no way to rematch on options change (since there
   // is no rule), so passing anything other than all_options is most likely a
@@ -446,7 +447,8 @@ namespace build2
                 uint64_t options = match_extra::all_options);
 
   // Match (but do not apply) the specified rule directly and without
-  // incrementing the dependency counts. The target must be locked.
+  // incrementing the dependency counts. The target must be locked (and it
+  // remains locked after this function returns).
   //
   void
   match_rule (target_lock&,
diff --git a/libbuild2/cc/common.cxx b/libbuild2/cc/common.cxx
index 6d36099..99c8f5d 100644
--- a/libbuild2/cc/common.cxx
+++ b/libbuild2/cc/common.cxx
@@ -1472,7 +1472,7 @@ namespace build2
         // @@ TODO: we currently always reload pkgconfig for lt (and below).
         //
         mark_cc (*lt);
-        lt->mtime (mt);
+        lt->mtime (mt); // Note: problematic, see below for details.
 
         // We can only load metadata from here since we can only do this
         // during the load phase. But it's also possible that a racing match
@@ -1523,6 +1523,9 @@ namespace build2
         return l;
       };
 
+      target_lock al (lock (a));
+      target_lock sl (lock (s));
+
       target_lock ll (lock (lt));
 
       // Set lib{} group members to indicate what's available. Note that we
@@ -1550,9 +1553,6 @@ namespace build2
           ll.unlock ();
       }
 
-      target_lock al (lock (a));
-      target_lock sl (lock (s));
-
       if (!al) a = nullptr;
       if (!sl) s = nullptr;
 
@@ -1586,6 +1586,34 @@ namespace build2
       if (s != nullptr) match_rule (sl, file_rule::rule_match);
       if (ll)
       {
+        // @@ Turns out this has a problem: file_rule won't match/execute
+        //    group members. So what happens is that if we have two installed
+        //    libraries, say lib{build2} that depends on lib{butl}, then
+        //    lib{build2} will have lib{butl} as a prerequisite and file_rule
+        //    that matches lib{build2} will update lib{butl} (also matched by
+        //    file_rule), but not its members. Later, someone (for example,
+        //    the newer() call in append_libraries()) will pick one of the
+        //    members assuming it is executed and things will go sideways.
+        //
+        //    For now we hacked around the issue but the long term solution is
+        //    probably to add to the bin module a special rule that is
+        //    registered on the global scope and matches the installed lib{}
+        //    targets. This rule will have to both update prerequisites like
+        //    the file_rule and group members like the lib_rule (or maybe it
+        //    can skip prerequisites since one of the member will do that; in
+        //    which case maybe we will be able to reuse lib_rule maybe with
+        //    the "all members" flag or some such). A few additional
+        //    notes/thoughts:
+        //
+        //    - Will be able to stop inheriting lib{} from mtime_target.
+        //
+        //    - Will need to register for perform_update/clean like in context
+        //      as well as for configure as in the config module (feels like
+        //      shouldn't need to register for dist).
+        //
+        //    - Will need to test batches, immediate import thoroughly (this
+        //      stuff is notoriously tricky to get right in all situations).
+        //
         match_rule (ll, file_rule::rule_match);
 
         // Also bless the library group with a "trust me it exists" timestamp.
@@ -1594,6 +1622,8 @@ namespace build2
         // won't match.
         //
         lt->mtime (mt);
+
+        ll.unlock (); // Unlock group before members, for good measure.
       }
 
       return r;
diff --git a/libbuild2/cc/link-rule.cxx b/libbuild2/cc/link-rule.cxx
index 499f867..1991eda 100644
--- a/libbuild2/cc/link-rule.cxx
+++ b/libbuild2/cc/link-rule.cxx
@@ -2265,6 +2265,29 @@ namespace build2
                      << "for install";
           }
 
+          auto newer = [&d, l] ()
+          {
+            // @@ Work around the unexecuted member for installed libraries
+            // issue (see search_library() for details).
+            //
+            // Note that the member may not even be matched, let alone
+            // executed, so we have to go through the group to detect this
+            // case (if the group is not matched, then the member got to be).
+            //
+#if 0
+            return l->newer (d.mt);
+#else
+            const target* g (l->group);
+            target_state s (g != nullptr                           &&
+                            g->matched (d.a, memory_order_acquire) &&
+                            g->state[d.a].rule == &file_rule::rule_match
+                            ? target_state::unchanged
+                            : l->executed_state (d.a));
+
+            return l->newer (d.mt, s);
+#endif
+          };
+
           if (d.li.type == otype::a)
           {
             // Linking a utility library to a static library.
@@ -2292,7 +2315,7 @@ namespace build2
             // Check if this library renders us out of date.
             //
             if (d.update != nullptr)
-              *d.update = *d.update || l->newer (d.mt);
+              *d.update = *d.update || newer ();
 
             for (const target* pt: l->prerequisite_targets[d.a])
             {
@@ -2331,7 +2354,7 @@ namespace build2
             // Check if this library renders us out of date.
             //
             if (d.update != nullptr)
-              *d.update = *d.update || l->newer (d.mt);
+              *d.update = *d.update || newer ();
 
             // On Windows a shared library is a DLL with the import library as
             // an ad hoc group member. MinGW though can link directly to DLLs
@@ -3497,7 +3520,7 @@ namespace build2
                                 &cs, &update, mt,
                                 bs, a, *f, la, p.data, li,
                                 for_install, true, true, &lc);
-              f = nullptr; // Timestamp checked by hash_libraries().
+              f = nullptr; // Timestamp checked by append_libraries().
             }
             else
             {
diff --git a/libbuild2/target.ixx b/libbuild2/target.ixx
index 2e1c65e..39b81e7 100644
--- a/libbuild2/target.ixx
+++ b/libbuild2/target.ixx
@@ -769,19 +769,7 @@ namespace build2
   inline bool mtime_target::
   newer (timestamp mt, target_state s) const
   {
-#ifndef NDEBUG
-    // @@ TMP
-    //
-    if (s == target_state::unknown)
-    {
-      text << "unknown target_state in newer(): " << *this <<
-        info << "phase: " << ctx.phase;
-
-      terminate (true /* trace */);
-    }
-
     assert (s != target_state::unknown); // Should be executed.
-#endif
 
     timestamp mp (mtime ());