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.

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

Refactoring target for TriBITS and packages using TriBITS #342

Open
bartlettroscoe opened this issue Dec 9, 2020 · 5 comments
Open

Refactoring target for TriBITS and packages using TriBITS #342

bartlettroscoe opened this issue Dec 9, 2020 · 5 comments

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Dec 9, 2020

This story is to capture a design discussion about the refactoring target for TriBITS and projects/packages that use TriBITS.

Blocks: #63, #299

Related to:

  • SEPW-214
@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Dec 9, 2020

CC: @sethrj

Had a good conversations with @jjwilke and @keitat about the desired goals and targets for refactoring of TriBITS and for getting some external help with the refactorings. I will create a wiki page somewhere that fleshes out the refactoring targets but in summary:

  • TriBITS packages should be able to use raw CMake for everything internally to create libraries, etc. They would just be required to create the targets described in this proposed standard for <Package>Config.cmake files.
  • TriBITS packages would still be expected to call tribits_add_test(), tribits_add_advanced_test() and even tribits_add_executable_and_test() to define tests.
  • Finding external packages (i.e. TPLs) would still be handed over to TriBITS to call find_package(), even when a TriBITS package is pulled out and built as its own CMake project.
  • The TriBITS refactoring allowing all of this should allow existing TriBITS packages (e.g. all of Trilinos, Drekar, Charon2, all of the VERA packages) to be upgraded with (near) zero modifications.
  • The decision to use TriBITS functions like tribits_add_library() and related internal target-building commands should be a local decision and some/many Trilinos packages will decide to move to raw CMake (or thin local wrapper functions) for their local CMake builds.

Again, I will start a wiki page somewhere that fleshes this out more and get feedback on this.

NOTE: With Trilinos and Kokkos soon upgrading to CMake 3.17 (see trilinos/Trilinos#8401), this will be feasible.

@bartlettroscoe
Copy link
Member Author

@fnrizzi, @marcinwrobel1986, @MikolajZuzek

FHI: This is a prerequisite for completing #63 and #299. That a lot of work but important that we all agree on these targets.

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Aug 25, 2021

FYI: I made some small edits to the proposed standard for Config.cmake files based on an email conversation with @ibaned about that proposed standard and its relation to https://github.com/sandialabs/capp. Specifically, I removed the spec that packages should use CamelCase and mentioned that packages can choose to provide <lower-case-package-name>-config.cmake instead of <PackageName>Config.cmake (noting that package_name(<PackageName>) will find either of these files).

@bartlettroscoe
Copy link
Member Author

With @ibaned's permission, the contents of our email chain is given below. Just my last email response is in open text. The rest of the chain is collapsed by default.

Thanks for the feedback and the discussion @ibaned!


From: Bartlett, Roscoe A
Sent: Wednesday, August 25, 2021 5:55 PM
To: Ibanez, Daniel Alejandro daibane@sandia.gov
Subject: RE: CCR Seminar: Dan Ibanez- Better Package Management

Hello Dan,

Posting the email thread is fine with me!

Thanks, I will post this reply and the rest of this email chain in that GitHub.

I can see how globbing avoids frequent complaints for a piece of software that doesn’t have great installation testing. My hope is that as people adopt package managers like Spack or CApp that have installation as a routine part of their functionality (i.e. packages only depend on upstream install trees, not build or source trees), this will in effect provide much more widespread installation testing (in the CApp workflow, installation testing is performed before submitting a change to a package).

Installation is great for usage of a package but is terrible as a required step in the iterative development of a package. And having to manually list your files is just more grunt work. To each their own I guess.

As far as reconfiguration, the projects which follow the guidelines in my slides have always reconfigured well. Conversely, I’ve had to add a NO_CONFIGURE_CACHE option in CApp for projects which don’t reconfigure well, and this is activated for Trilinos and SEACAS.

Then you are getting lucky with some CMake projects or they have some super simplistic CMakeLists.txt files. For example, all you need is to have (perfectly reasonable) CMake code like this:

set(CACHE_VAR_1 ON CACHE BOOL “…”)
set(CACHE_VAR_2 ${CACHE_VAR_1} CACHE BOOL “…”)

and you can no longer trust re-configures. If someone changes the value for CACHE_VAR_1 and it will not change the value of CACHE_VAR_2 on a reconfigure. You can’t even trust a simple single set() statement like:

set(CACHE_VAR ON CACHE BOOL “…”)

If someone does the initial configure (setting the value of CACHE_VAR to ON in CMakeLists.txt), the modifies that line to change the default value from ‘ON’ to ‘OFF’ and then reconfigures, they will get a different configuration than if they configured from scratch. Kaboom! CMake is just fundamentally broken with respect to reconfigures due to the way that cache vars work. You can never fully trust a reconfigure for arbitrary changes to the CMakeLists.txt files or to changes in the system, period. It is just bat &*T@ crazy to trust reconfigures, period.

I definitely remove build directories manually from time to time, but never out of strict necessity for my projects, more of an old habit dying hard.

NOTE: I never said anything about deleting the build directory. If you use CMake correctly and you run ‘rm -r CMake*’ and then reconfigure an existing build dir, you can rebuild a build directory over and over again and get correct builds, every time. We had dozens ATDM Trilinos builds on many different platforms that rebuilt correctly for two years straight with no problems! (It is actually fragile test suites in Trilinos that can make rebuilds a problem due to old files sitting around.)

I definitely hope that Trilinos packages start to shift to the CMake equivalent of duplicated features and TriBITS boils down to its essence as you say, and I have a dream of a future where each Trilinos package is its own repository because most Trilinos users don’t use most Trilinos packages.

I would be all for each Trilinos package having its own git repo (and I actually argued for that back in 2016 when I came back to SNL from ORNL given the success of multi-repo CASL) but the reality is that multi-repo git is way to complicated for most Trilinos developers and users at SNL and they are not willing to adopt something simple like gitdist.

Whether any of that comes true depends on many factors, but it sounds like our thoughts on CMake are pretty well aligned which gives me hope that we are walking in the right direction.

Indeed. Since you have been thinking about this problem for a while as well and you also see similar challenges with the over-the-top Spack approach (i.e. the crazy-complicated concretization algorithm that is nearly impossible to control or debug) it would be good if we could try to coordinate in some way to try to converge these efforts or make sure they are as mutually compatible as much as possible (i.e. TriBITS packages can be built and deployed with CApp or packages that work with CApp can be easily cast as TriBITS packages in a TriBITS meta-project).

I look forward to your talk next Wednesday,

Cheers,

-Ross

P.S. I have been reading the book Professional CMake 5th Edition and it seems to be explaining modern CMake pretty well. There are some important things that are not well explained in that book or in official CMake documentation that someone needs to document at some point like the exact details and specs for the handling of compiler and linker options (see #375). The updated documentation does not seem to be enough from what I can see to really explain this, at least not to my satisfaction.


From: Ibanez, Daniel Alejandro Sent: Wednesday, August 25, 2021 3:34 PM To: Bartlett, Roscoe A Subject: Re: CCR Seminar: Dan Ibanez- Better Package Management Importance: High

Hey Ross,

Posting the email thread is fine with me!

Thanks for recording that change, flexible file naming should make things easier.

I understand the motivation for “all_libs” and will be happy to create that target for packages I’m responsible for. My de facto practice right now is to manually link to upstream targets, i.e. packages need to know both the package names and target names of their upstream dependencies.

I can see how globbing avoids frequent complaints for a piece of software that doesn’t have great installation testing. My hope is that as people adopt package managers like Spack or CApp that have installation as a routine part of their functionality (i.e. packages only depend on upstream install trees, not build or source trees), this will in effect provide much more widespread installation testing (in the CApp workflow, installation testing is performed before submitting a change to a package).

As far as reconfiguration, the projects which follow the guidelines in my slides have always reconfigured well. Conversely, I’ve had to add a NO_CONFIGURE_CACHE option in CApp for projects which don’t reconfigure well, and this is activated for Trilinos and SEACAS. I definitely remove build directories manually from time to time, but never out of strict necessity for my projects, more of an old habit dying hard.

I definitely hope that Trilinos packages start to shift to the CMake equivalent of duplicated features and TriBITS boils down to its essence as you say, and I have a dream of a future where each Trilinos package is its own repository because most Trilinos users don’t use most Trilinos packages. Whether any of that comes true depends on many factors, but it sounds like our thoughts on CMake are pretty well aligned which gives me hope that we are walking in the right direction.

Thank you,

Dan

From: "Bartlett, Roscoe A" Date: Wednesday, August 25, 2021 at 11:51 AM To: "Ibanez, Daniel Alejandro" Subject: RE: CCR Seminar: Dan Ibanez- Better Package Management

Hello Dan,

Never mind about commenting in the Google Doc. I went ahead and took out the CamelCase requirement (there is no need for it) and added the note:

• Packages may also provide lower-case version of file package config file <lower-case-package-name>-config.cmake but that is not necessary given that find_package(<PackageName>) will automatically search for <lower-case-package-name>-config.cmake.

Given that find_package() finds either version of the package-config file, this is really a preference.

I will note that you can’t suggest people use the idiom Foo::foo for the set of targets produced because different existing packages define this target in inconsistent ways. You need to define a new target called something like ‘all_libs’ and (obviously) that target can’t contain main() (like with with the ‘gtest_main’ target). If you don’t do that, you can’t implement something like something like the existing TriBITS auto-package lib linking (which is used in thousands of CMakeLists.txt files). If each package does manual linking to upstream target libraries (which will be allowed in the next refactoring of TriBITS), then this is not as much of an issue (but still it will avoid confusion about what these targets mean).

I disagree about the statement about not globing source files (especially header files). If every project had strong installation testing, then it would be okay to not glob but no project has good installation testing. Otherwise, globing source and header files in Trilinos is about the only reason why installs of Trilinos are not broken more often than they already are. The argument that manually listing source and header files leads to correct auto-reconfigure is a weak argument. The reality is that you never 100% trust a correct configuration of a CMake project by just typing ‘make’ and expecting the CMake reconfigure with an existing CMakeCache.txt file (I can give several reasons for this that are fundamental to CMake). As a developer convenience, auto-reconfigure is nice, but when there is any doubt, everyone knows you have to run ‘rm -r CMake*’ and the configure from scratch if you want to be sure your rebuild is correct.

Otherwise, I agree with everything in your presentation. I will note that the goal of the current TriBITS refactorings underway are to:

  • Allow these CMake packages to be aggregated into arbitrary collections of CMake projects to be built, tested and installed
  • Allow CMake packages optionally use TriBITS functions internally or raw CMake commands to define targets, etc. (as long as they follow basic standards for definition of targets and tests)

A lot of functionality was put in TriBITS in the early years that has since been duplicated in CMake in later years. It will be a major effort to refactor Trilinos and other codes currently using TriBITS to use the newer native CMake implementations of this functionality and trim TriBITS to the bare minimum of what it should be (which is a flexible CMake package manager, efficient multi-package development driver, and optional set of macros/functions to streamline the creation of CMake packages).

Cheers,

-Ross

P.S. Do you mind if I post this email thread to #342 to document it?

From: Bartlett, Roscoe A Sent: Wednesday, August 25, 2021 12:56 PM To: Ibanez, Daniel Alejandro Subject: RE: CCR Seminar: Dan Ibanez- Better Package Management

Hello Dan,

Do you mind commenting in the Google Doc itself so we can archive it there and others can see your comments? If not, I can respond in email.

-Ross

From: Ibanez, Daniel Alejandro Sent: Wednesday, August 25, 2021 12:54 PM To: Bartlett, Roscoe A Subject: Re: CCR Seminar: Dan Ibanez- Better Package Management Importance: High

Hi Ross,

Most of that spec seems pretty sensible to me.

One thing I would remove is the requirement for camel case naming of packages, I personally use package-name-config.cmake files and all lowercase package names.

I’m fine designating some standard target name like “all_libs” for “the typical set of desired libraries”. The definition “all of the imported library targets” is problematic, because for example Gtest exports targets “gtest” and “gtest_main” which are mutually exclusive. But I get the motivation that guessing target names is hard. Maybe I would suggest the additional requirement that packages provide documentation of their exported CMake targets.

Using find_dependency is an important requirement that I’ve only recently started to obey but quite like.

I’ve attached the slides which explain in detail what I plan to suggest that people do with CMake. I think you’ll find it’s fairly compatible with your spec, although not everything in the spec is talked about in the slides. I think we both agree that standardization of how we use CMake for packages is required for a bright future in package management.

Thanks!
Dan

From: "Bartlett, Roscoe A" Date: Wednesday, August 25, 2021 at 10:42 AM To: "Ibanez, Daniel Alejandro" Subject: FW: CCR Seminar: Dan Ibanez- Better Package Management

Hello Dan,

Just saw this. Does your approach address the problem of no defined standard for the contents of Config.cmake files? Can you please take a look at our proposed standard for this that we worked with Kitware currently in draft form at:

https://docs.google.com/document/d/19gIZgPAfnK7RTdh0sMKr5Yeo1epPVKIfona-oxoCtsg/edit#heading=h.6v29dm1zgr8x

?

If everyone could agree on these standard targets and behaviors, it would massively improve CMake package management (and TriBITS-based packages could seamlessly interoperate with native CMake packages, etc.).

If you can find some time, please let me know (that is a pretty short spec).

Thanks,

-Ross

@bartlettroscoe
Copy link
Member Author

FYI: I implemented the <Project>::all_libs and <Package>::all_libs INTERFACE library targets in the <Project>Config.cmake and the <Package>Config.cmake files as per this proposed standard for Config.cmake files. As for the package-level files <Project>Config.cmake I was able to do this for both files put in the build tree and the install tree and ALIAS targets <Package>::all_libs also exist in the native CMake project.

In this context, for the file <Project>Config.cmake, the "Components" are the individual TriBITS packages. Note that I did not implement the targets <Project>::<cpi>_libs (which map to <Package>::all_libs for each package) as proposed by Jeremy in that specification but I could do so easily.

Note that I extended the specification in the Google Doc by adding an optional <PackageName>::all_selected_libs target to match the COMPONENTS argument. That makes it easier to link against all of the libraries in all of the selected components. That made it easier to implement the example that I had and was easy to implement and support in TriBITS.

There is more work to do with the handling of external packages/TPLs and how that impacts the <Project>Config.cmake especially before I can declare success but at least this is good progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

1 participant