Skip to content

[roottest] Fix build order for root/tree/address tests#19716

Merged
devajithvs merged 1 commit intoroot-project:masterfrom
devajithvs:macbeta-fix
Aug 21, 2025
Merged

[roottest] Fix build order for root/tree/address tests#19716
devajithvs merged 1 commit intoroot-project:masterfrom
devajithvs:macbeta-fix

Conversation

@devajithvs
Copy link
Contributor

@devajithvs devajithvs commented Aug 21, 2025

This restores the behaviour of the old Makefile rule:

ConfigRecord_cxx.$(DllSuf): ConfigRecord.cxx ConfigRecord.h sueloader_C.$(DllSuf)
    $(BuildWithLib)

which ensured sueloader_C.so was available before linking ConfigRecord_cxx.so.

This Pull request:

Changes or fixes:

Test failures on mac beta.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

This restores the behaviour of the old Makefile rule:

    ConfigRecord_cxx.$(DllSuf): ConfigRecord.cxx ConfigRecord.h sueloader_C.$(DllSuf)
        $(BuildWithLib)

which ensured sueloader_C.so was available before linking ConfigRecord_cxx.so.
@devajithvs devajithvs self-assigned this Aug 21, 2025
@devajithvs devajithvs requested a review from bellenot as a code owner August 21, 2025 14:58
@devajithvs devajithvs requested a review from linev August 21, 2025 14:58
@couet couet self-requested a review August 21, 2025 15:00
@devajithvs devajithvs added the clean build Ask CI to do non-incremental build on PR label Aug 21, 2025
@devajithvs devajithvs requested review from dpiparo and pcanal August 21, 2025 15:45
@github-actions
Copy link

github-actions bot commented Aug 21, 2025

Test Results

    21 files      21 suites   3d 15h 3m 47s ⏱️
 3 560 tests  3 559 ✅ 0 💤 1 ❌
73 007 runs  73 006 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 8c34b9f.

♻️ This comment has been updated with latest results.

@devajithvs devajithvs merged commit c34d1d6 into root-project:master Aug 21, 2025
46 of 50 checks passed
@devajithvs devajithvs deleted the macbeta-fix branch August 21, 2025 18:33
ROOTTEST_COMPILE_MACRO(ConfigRecord.cxx
FIXTURES_SETUP root-tree-addresses-ConfigRecord-fixture)
FIXTURES_SETUP root-tree-addresses-ConfigRecord-fixture
FIXTURES_REQUIRED root-tree-addresses-sueloader-fixture)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? How does the generation of sueloader_C affects the generation of ConfigRecord_cxx?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devajithvs ping?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I responded to the above as a separate comment below!

I just wanted to clarify this and I will open a new PR reverting this and a proper fix:

Are these symbols expected to remain unresolved here and instead come from another library later?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing the error message, it is clear that we missed bringing to the CMakeFile from the Makefile the part that linked ConfigRecord_cxx against sueloader_C. So this change is correct but incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will close the other PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@devajithvs
Copy link
Contributor Author

devajithvs commented Aug 22, 2025

After some digging, I realized this change was only fixing the symptom, not the underlying issue. I will open a new PR reverting this and a proper fix.

You can reproduce the same behaviour in master (and even before the move from makefile based tests to cmake):

$BUILD/bin/root.exe -e "gSystem->SetBuildDir(\"$BUILD/roottest/root/tree/addresses\", true)" -q -l -b "$ROOT/roottest/scripts/build.C(\"$ROOT/roottest/root/tree/addresses/ConfigRecord.cxx\",\"\",\"\")"

Processing /Users/sftnight/dvalapar/root/roottest/scripts/build.C("/Users/sftnight/dvalapar/root/roottest/root/tree/addresses/ConfigRecord.cxx","","")...
Info in <TMacOSXSystem::ACLiC>: creating shared library /Users/sftnight/dvalapar/root/builddir/roottest/root/tree/addresses/ConfigRecord_cxx.so
Undefined symbols for architecture arm64:
  "RecRecordImp<RecHeader>::Class()", referenced from:
      RecRecordImp<RecHeader>::IsA() const in ConfigRecord_cxx_ACLiC_dict.o
      RecRecordImp<RecHeader>::ShowMembers(TMemberInspector&) const in ConfigRecord_cxx_ACLiC_dict.o
  "RecRecordImp<RecHeader>::Streamer(TBuffer&)", referenced from:
      vtable for RecRecordImp<RecHeader> in ConfigRecord_cxx_ACLiC_dict.o
  "RecHeader::Print(char const*) const", referenced from:
      RecRecordImp<RecHeader>::Print(char const*) const in ConfigRecord_cxx_ACLiC_dict.o
  "vtable for Context", referenced from:
      RecRecordImp<RecHeader>::RecRecordImp() in ConfigRecord_cxx_ACLiC_dict.o
   NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
  "vtable for RecHeader", referenced from:
      ConfigRecord::~ConfigRecord() in ConfigRecord_cxx_ACLiC_dict.o
      ConfigRecord::~ConfigRecord() in ConfigRecord_cxx_ACLiC_dict.o
      ROOT::deleteArray_ConfigRecord(void*) in ConfigRecord_cxx_ACLiC_dict.o
      RecRecordImp<RecHeader>::RecRecordImp() in ConfigRecord_cxx_ACLiC_dict.o
      RecRecordImp<RecHeader>::~RecRecordImp() in ConfigRecord_cxx_ACLiC_dict.o
      RecRecordImp<RecHeader>::~RecRecordImp() in ConfigRecord_cxx_ACLiC_dict.o
   NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
ld: symbol(s) not found for architecture arm64

How does the generation of sueloader_C affects the generation of ConfigRecord_cxx?

When we had sueloader_C.so, these symbols were resolved from that library (at least in incremental builds).

This could be fixed by including the right headers, but I’m not entirely clear on what this test is doing. @pcanal Are these symbols expected to remain unresolved here and instead come from another library later?

This issue only affects macOS. On Linux the linker allows this by default. On macOS, we need the flags: -Wl,-undefined,dynamic_lookup on macOS for linking to succeed. We could do something like what was done here: https://github.com/root-project/root/pull/11432/files

diff --git a/core/base/src/TSystem.cxx b/core/base/src/TSystem.cxx
index 7b169455cca..05979f06950 100644
--- a/core/base/src/TSystem.cxx
+++ b/core/base/src/TSystem.cxx
@@ -3828,6 +3828,10 @@ int TSystem::CompileMacro(const char *filename, Option_t *opt,
          ForeachSharedLibDep(library, CollectF);
 
          TString relink_cmd = cmd.Strip(TString::kTrailing, ';');
+#ifdef R__MACOSX
+         // Allow linking to succeed despite the missing symbols.
+         relink_cmd.ReplaceAll("-dynamiclib", "-dynamiclib -Wl,-w -Wl,-undefined,dynamic_lookup");
+#endif
          relink_cmd += depLibsFullPaths;
          if (verboseLevel > 3 && withInfo) {
             ::Info("ACLiC", "relinking against all dependencies");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clean build Ask CI to do non-incremental build on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments