Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[core] ACLiC: speed up building / loading: #8017

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[core] ACLiC: speed up building / loading: #8017

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9595cce
[core] ACLiC: speed up building / loading:
Axel-Naumann Apr 27, 2021
ed10012
[core] Deprecate !(ACLiC.LinkLibs & 1). Remove ancient macOS case.
Axel-Naumann Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions core/base/src/TSystem.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ allows a simple partial implementation for new OS'es.
*/

#include <ROOT/FoundationUtils.hxx>
#include <ROOT/StringUtils.hxx>
#include "strlcpy.h"
#include "TSystem.h"
#include "TApplication.h"
Expand Down Expand Up @@ -3316,15 +3317,17 @@ int TSystem::CompileMacro(const char *filename, Option_t *opt,
#endif
Bool_t linkDepLibraries = !produceRootmap;
if (gEnv) {
#if (defined(R__MACOSX) && !defined(MAC_OS_X_VERSION_10_5))
Int_t linkLibs = gEnv->GetValue("ACLiC.LinkLibs",2);
#elif defined(R__WIN32)
#if defined(R__WIN32)
Int_t linkLibs = gEnv->GetValue("ACLiC.LinkLibs",3);
#else
Int_t linkLibs = gEnv->GetValue("ACLiC.LinkLibs",1);
#endif
produceRootmap = linkLibs & 0x2;
linkDepLibraries = linkLibs & 0x1;
if (!linkDepLibraries) {
static int onetime = (Warning("ACLiC", "Unsetting `ACLiC.LinkLibs & 1` is deprecated!"), 1);
(void)onetime;
}
}

// FIXME: Triggers clang false positive warning -Wunused-lambda-capture.
Expand All @@ -3346,16 +3349,19 @@ int TSystem::CompileMacro(const char *filename, Option_t *opt,
if (!f(I->c_str()))
break;
};
auto LoadLibrary = [useCxxModules, produceRootmap, ForeachSharedLibDep](const TString &lib) {

auto LoadLibrary = [linkDepLibraries, useCxxModules, produceRootmap, ForeachSharedLibDep](const TString &lib) {
// We have no rootmap files or modules to construct `-l` flags enabling
// explicit linking. We have to resolve the dependencies by ourselves
// taking the job of the dyld.
// If !linkDepLibraries then the library might not know its dependencies
// and "just load the library" might fail.
// FIXME: This is a rare case where we have rootcling running with
// modules disabled. Remove this code once we fully switch to modules,
// or implement a special flag in rootcling which selective enables
// modules for dependent libraries and does not produce a module for
// the ACLiC library.
if (useCxxModules && !produceRootmap) {
if (!linkDepLibraries && useCxxModules && !produceRootmap) {
std::function<bool(const char *)> LoadLibF = [](const char *dep) {
return gInterpreter->Load(dep, /*skipReload*/ true) >= 0;
};
Expand All @@ -3374,8 +3380,6 @@ int TSystem::CompileMacro(const char *filename, Option_t *opt,
if (!keep) k->SetBit(kMustCleanup);
fCompiled->Add(k);

gInterpreter->GetSharedLibDeps(library);
Copy link
Member

@pcanal pcanal Nov 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This call is still needed for the not-explicitly-linked case. It has the side effect of load the dependencies from the rootmap files so that the subsequent LoadLibrary can use that information. See commit 8fdd5c3

Copy link
Member

Choose a reason for hiding this comment

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

Related we should probably deprecate the option to disable explicit linking in ACLiC (it is already removed from the main build). Eitherway removing this line probably require to add (or check we already have) a test for the problem solved by commit 8fdd5c3


return LoadLibrary(library);
}
else return kTRUE;
Expand Down
Loading