search

TriBITSPub / TriBITS

TriBITS: Tribal Build, Integrate, and Test System,
http://tribits.org
Other
36 stars 46 forks source link

Updated TriBITS is disabling downstream TPLs #557

Closed bartlettroscoe closed 7 months ago

bartlettroscoe commented 1 year ago

Description

The updated TriBITS snapshotted to Trilinos in PR https://github.com/trilinos/Trilinos/pull/11380 has broken backward compatibility in that disabling an upstream TPL like HDF5 will disable a downstream TPL like NetCDF. Well, that breaks some configure scripts for Trilinos like the one for Albany as reported in https://github.com/trilinos/Trilinos/issues/11426.

We, it turns out that I knew that was going to happen as I documented it in the tribits/CHANGLOG.md 2022-10-20 entry:

Changed: Disabling an external package/TPL will now disable any downstream external packages/TPLs that list a dependency on that external package/TPL through its FindTPL<tplName>Dependencies.cmake file. Prior to this, disabling an external package/TPL would not disable dependent downstream external packages/TPLs (it would only disable downstream dependent required internal packages). To avoid this, simply leave the enable status of the upstream external package/TPL empty "" and no downstream propagation of disables will take place.

which you can see got posted in the PR in the file tribits/CHANGLOG.md.

Proposed solution

I think the proposed solution, for now, is to make TPL dependencies optional instead of required. Such dependencies already behave like they are optional in that enabling a TPL like NetCDF will not automatically enable a dependent upstream TPL like HDF5. To provide maximum flexibility in how people define TPLs, we should likely make all TPL dependencies optional.

NOTE: The reason that TPL dependencies had to be added was to correctly propagate and order TPL libraries to downstream targets. TPL dependencies were not added to change enable/disable behavior. (Not sure why I decided to make TPL dependencies behave this way.)

bartlettroscoe commented 7 months ago

This was resolved with the merge of the PR: