[core] circumvent call to rmkdepend by ferdymercury · Pull Request #23492 · root-project/root

ferdymercury · 2026-09-25T07:06:58Z

maybe it improves performance a bit

and one could get rid of rmkdepend dependency / hard-coded builtin

AI-assisted

not yet fully working

AI-assisted

github-actions · 2026-09-25T13:59:04Z

Test Results

23 files 23 suites 3d 12h 32m 29s ⏱️
3 883 tests 3 680 ✅ 29 💤 174 ❌
72 730 runs 68 645 ✅ 651 💤 3 434 ❌

For more details on these failures, see this check.

Results for commit e3e4e3d.

♻️ This comment has been updated with latest results.

warning: treating 'c-header' input as 'c++-header' when in C++ mode, this behavior is deprecated [-Wdeprecated]

otherwise one needs to fall back to rmkdepend for WIN32

as suggested by pcanal

ferdymercury · 2026-09-30T10:33:24Z

@bellenot could you check on Windows why this error is printed in the CI?

Failed to close pipe dependencies for ...
Maybe some flag in the reconstructed command is incorrect?
Thanks in advance.

ferdymercury · 2026-09-30T16:42:36Z

unix failures are unrelated.

win32 failure are related, but only 6 tests failing, so not too bad.

rm later when fixed

dpiparo · 2026-10-09T18:32:30Z

@ferdymercury do you need help to push this through, e.g. by @bellenot ?

bellenot · 2026-10-09T19:08:13Z

I'll take a look

pcanal · 2026-10-09T21:47:58Z

-   // Generate the dependency via standard output, not searching the
-   // standard include directories,
+#ifdef WIN32
+   TString compiler = "\"" COMPILER "\""; // 'Program Files' contains spaces on Windows


Usually we use the CXX (actually indirectly) from compiledata.h and this does not usually contain spaces (it is the basename of the compiler).

I think this is slightly safer in case it changes later on?

It is a two edges sword. We had to go with the basename because the same version of the compiler is not always installed in the same location on all system (but is almost always in the path). When we used the full path name we failed unless the binary was installed in the exact same configuration/distribution. This is critical for the experiment software. Even for Windows this is needed since the user can elect to install the compiler in a different place (for example on a different disk for space reason).

pcanal · 2026-10-09T21:49:17Z

+   cmd += " -MF \"" + depfilename + "\"";
+   cmd += " -MT \"";
+   R__AddPath(cmd, targetname);
+   cmd += "\" ";


The design of ACLiC here was to delegate this whole command line build (for the most part) to the build system and insert it in compiledata.h

For example we have:

#define MAKESHAREDLIB "cd $BuildDir ; c++ -fPIC -c $Opt -Wc++11-narrowing -Wsign-compare -Wsometimes-uninitialized -Wconditional-uninitialized -Wheader-guard -Warray-bounds -Wcomment -Wtautological-compare -Wstrncat-size -Wloop-analysis -Wbool-conversion -m64 -pipe -W -Woverloaded-virtual -fsigned-char -fsized-deallocation -fno-common -Qunused-arguments -pthread -stdlib=libc++ -std=c++17 $IncludePath $SourceFiles ; c++ $Opt $ObjectFiles -dynamiclib -Wl,-headerpad_max_install_names -m64 -Wl,-dead_strip_dylibs -Wl,-install_name,@rpath/$LibName.so $RPath $LinkedLibs -o $SharedLib "

So I would imagine we would add a new line in compiledata.h with all the compiler/OS specific part filed in and the run-time part easily inserted via some $variables

Since this PR is a bit complicated, now that the CI is green, I'd prefer doing this optimization in a subsequent one. On the other hand, only TSystem would be using this, so I do not see that it simplifies things. The ifdefs in the Cxx would be just moved to the CMake code, so similar if not larger number of lines and more abstraction. If there would be more places being used, then I would see that more clearly.

Yes, it sortof can wait for a further PR. Yes, the complexity is essentially the same.

There is 2 major distinctions

One is to have the same pattern for all the cases rather than 2 different ways.

Additionally, going through the indirection of having one command line pattern allows the user to manipulate and customizing as they need.

bellenot · 2026-10-12T14:37:58Z

@ferdymercury With my suggestions:

1/7 Test #1744: roottest-root-aclic-misc-assertROOT7027 ........   Passed    9.61 sec
2/7 Test #1740: roottest-root-aclic-load-reload ................   Passed   18.07 sec
3/7 Test #2387: roottest-root-io-newstl-buildPospelov ..........   Passed   21.23 sec
    Start 2388: roottest-root-io-newstl-printPospelov
4/7 Test #2388: roottest-root-io-newstl-printPospelov ..........   Passed    6.43 sec
5/7 Test  #105: gtest-core-metacling-TClingLoadUnloadFile ......   Passed   28.74 sec
6/7 Test  #945: tutorial-roofit-roofit-rf104_classfactory ......   Passed   29.06 sec
    Start 1289: tutorial-roofit-roofit-rf104_classfactory-py
7/7 Test #1289: tutorial-roofit-roofit-rf104_classfactory-py ...   Passed   32.60 sec

by bellenot and pcanal Co-authored-by: Bertrand Bellenot <bellenot@users.noreply.github.com>

ferdymercury · 2026-10-13T07:03:17Z

CI failure seems unrelated.

I don't know if someone wants to run some speed test benchmark of compiling some long script with AcLiC? Maybe @LukasBreitwieser ?

to allow relocation as suggested by pcanal also basename does not contain spaces in general, so no need for escaping

[core] circumvent call to rmkdep

7eb1f72

AI-assisted

ferdymercury added the skip code analysis Skip the code analysis CI steps for this PR, including verifying clang-formatting and running Ruff. label Sep 25, 2026

ferdymercury added 2 commits September 25, 2026 09:14

!fixup missed var

9495138

typo

17ad775

ferdymercury changed the title ~~[core] circumvent call to rmkdep~~ [core] circumvent call to rmkdepend Sep 25, 2026

ferdymercury added 2 commits September 25, 2026 10:01

min standard cxx flag

72d74be

silence unused vars warnings

4d9a281

ferdymercury added the AI-Assisted label Sep 25, 2026

ferdymercury requested a review from guitargeek September 25, 2026 08:37

ferdymercury marked this pull request as ready for review September 25, 2026 08:38

ferdymercury requested review from dpiparo and pcanal as code owners September 25, 2026 08:38

dpiparo self-assigned this Sep 25, 2026

ferdymercury added 2 commits September 29, 2026 09:32

[core] silence deprecation warning for header extension mismatch

307c67c

warning: treating 'c-header' input as 'c++-header' when in C++ mode, this behavior is deprecated [-Wdeprecated]

[nfc] formatting

5958676

ferdymercury added the pr:squash on merge label Sep 29, 2026

ferdymercury added 2 commits September 29, 2026 11:07

[core] attempt fallback implementation for Windows

c4e39f8

otherwise one needs to fall back to rmkdepend for WIN32

[win] try catch stdout and strip extra messages

56e746c

ferdymercury requested a review from silverweed September 29, 2026 13:05

[win32] missing header

e602185

ferdymercury mentioned this pull request Sep 29, 2026

[rmkdepend] mv to builtins folder and deprecate public installation #23463

Closed

pcanal reviewed Sep 29, 2026

View reviewed changes

Comment thread core/base/src/TSystem.cxx Outdated

ferdymercury added 2 commits September 30, 2026 09:13

[core] use same compiler as the one used to compile ROOT

9e680ac

as suggested by pcanal

pass standard option variable

7de514e

ferdymercury requested a review from bellenot as a code owner September 30, 2026 07:20

ferdymercury commented Sep 30, 2026

View reviewed changes

Comment thread core/base/CMakeLists.txt Outdated

ferdymercury added 3 commits September 30, 2026 09:22

Update core/base/CMakeLists.txt

5b61a6c

try escape quote via build interface

4c887c3

try with targetcompiledef

a248727

ferdymercury requested a review from pcanal September 30, 2026 08:11

try more fixes for win32

5f6b8ad

ferdymercury mentioned this pull request Sep 30, 2026

[misc] deprecate rmkdep and rm outdated windows scripts #23545

Merged

ferdymercury added 5 commits October 1, 2026 14:29

fix -I include syntax and EP for optimization on windows

29d1cf7

[DEBUG] win32 failures

347d5df

rm later when fixed

windows fixes

78d64e2

win32 try extra quotes

3b8d5af

try more fixes

93cf0d6

pcanal reviewed Oct 9, 2026

View reviewed changes

Comment thread core/base/src/TSystem.cxx

bellenot reviewed Oct 12, 2026

View reviewed changes

Comment thread core/base/src/TSystem.cxx Outdated

bellenot reviewed Oct 12, 2026

View reviewed changes

Comment thread core/base/src/TSystem.cxx Outdated

pcanal reviewed Oct 12, 2026

View reviewed changes

Comment thread core/base/src/TSystem.cxx

pcanal reviewed Oct 12, 2026

View reviewed changes

Comment thread core/base/src/TSystem.cxx

ferdymercury commented Oct 12, 2026

View reviewed changes

Comment thread core/base/src/TSystem.cxx

Apply suggestions from code review

f2a8fe0

by bellenot and pcanal Co-authored-by: Bertrand Bellenot <bellenot@users.noreply.github.com>

ferdymercury commented Oct 29, 2026

View reviewed changes

Comment thread core/base/src/TSystem.cxx Outdated

[core] use basename of the compiler instead of full path

e3e4e3d

to allow relocation as suggested by pcanal also basename does not contain spaces in general, so no need for escaping

Conversation

ferdymercury commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Uh oh!

ferdymercury commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ferdymercury commented Sep 30, 2026

Uh oh!

dpiparo commented Oct 9, 2026

Uh oh!

bellenot commented Oct 9, 2026

Uh oh!

pcanal Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

ferdymercury Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

pcanal Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

pcanal Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

ferdymercury Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

pcanal Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bellenot commented Oct 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ferdymercury commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ferdymercury commented Sep 25, 2026 •

edited

Loading

github-actions Bot commented Sep 25, 2026 •

edited

Loading

ferdymercury commented Sep 30, 2026 •

edited

Loading

ferdymercury commented Oct 13, 2026 •

edited

Loading