-
Notifications
You must be signed in to change notification settings - Fork 45
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
Learnings from the VTK Module Upgrade to modern CMake #337
Comments
Below, I give a conversation that we had about this that I want to archive here. (I have redacted some stuff that is not pertinent). From: Ben Boeckel On Fri, Oct 09, 2020 at 15:42:22 +0000, Johnson, Seth R. wrote:
I've found the same with VTK's upgrade to new CMake code. As long as your practical interface is via targets, everything should interoperate well. The VTK module system transports information for itself via properties on the targets and aren't special other than that. It can consume non-VTK-module targets just fine and non-VTK-module targets can consume VTK's module targets without having to buy-in on the module system itself. If you do, it can help with things like Python wrapping and such though (using VTK's wrapping tools). There's one nit that can't be expressed in usage requirements (since they're form "if depends_on(A) and depends_on(B) then add_extra_flag()") for the autoinit subsystem. This is handled by the Note that to truly work well, everything should be targets. External packages end up as imported targets. Note that this also means that the resulting install tree is a lot cleaner and less likely to have build-machine paths embedded. Also, I should note that not everyone on this thread has access to the ECP Slack at all. --Ben |
From: Ben Boeckel On Thu, Oct 15, 2020 at 22:40:55 +0000, Bartlett, Roscoe A wrote: [redacted]
Global variables are a problem because the namespace is constrained. One cannot easily nest two projects using tribits if they try to communicate and "fight" over which one "owns" global variables (or properties for that matter). VTK does communicate to itself via global properties, but it's an internal mechanism and any conflict is a conflict at the target level anyways (they're also namespaced so as to avoid conflicting with any user-level properties). Using targets also means that if you have the target, you have the information. No scope jumping or similar shenanigans is necessary.
They can be. See: VTK doesn't do this because the properties contain information needed at configure time and to do different variables for install and build configurations (primarily for header locations) this properly requires generator expressions. VTK has mechanisms to export the information manually though; it's not super complicated. --Ben |
From: Bartlett, Roscoe A Hello Ben, CCing Keita, Seth, and Jeremy in on this thread ... (I don't know how to conduct a detailed conversation like this on Slack).
Would you have some time in early Nov? If not, is there another Kitware staff member with knowledge of the VTK module system to which I could ask these questions?
TriBITS solves that problem by namespacing all variables by the package name such as https://tribits.org/doc/TribitsDevelopersGuide.html#tribits-package (which was violated by the usage of TriBITS in SCALE actually). For that matter, don't the names of targets need to be globally unique across all packages and modules? So don't you have to prefix them with the Package name So if you have to prefix targets and global variables, what is the real is the difference? Global and target properties have a disadvantage that you have to read them into a local var before you can use them in any CMake logic. Global vars don't have the problem and so the code is tighter and less verbose.
Right, that is also what TriBITS does, but with namespaced variables.
But again, the target name needs to be namespaces as well. A global var is a global var, there are no scope jumping issues. One issue that we will have with the TriBITS refactoring to use modern CMake is that is that a "package" and a "target" are not the same thing. A "package" is an aggregation of targets, global variables, and other things. We will create aggregate targets like
Thanks, -Ross |
From: Boeckel, Ben [redacted] [redacted] Everything after "One question that I have is why using targets for everything are" looks good to me. --Ben |
Relates to:
|
CC: @fnrizzi, @marcinwrobel1986, @MikolajZuzek FYI: Turns out that @ppebay was involved with the VTK project and knows a lot about VTK modules. He can help us distill what we can from the recent VTK Module refactoring effort. |
Are there any further questions I can help answer? The author and implementer is right here too :) . |
@mathstuf, thanks for the the offer. What is currently the best documentation describing the updated VTK Module system? Is it: ? |
Yes, that's the place to start. All of the "Module" pages here should be useful though. Note that nitty-gritty details are only documented in the implementation itself, not via API docs. |
@mathstuf, is there a small example project that uses VTK Modules that demonstrates how it works and allows you to play with it? For example, TriBITS has some of these under: |
A few things I see from just poking around (haven't run it locally yet):
Why is this the case? I question the composability of this right now. It seems that TriBITS completely takes over the project build rather than being able to build just a few bits with TriBITS, combine with a VTK module or two over here, pybind11 wrapping over there, and then tied together at the end. Maybe that's sufficient, but I think this looks hard to compose with other CMake APIs and such, but maybe the examples just aren't expressive enough here. Note that VTK's module system basically provides a toolbox with which to piece things together. Yes, this leaves a lot of boilerplate, but that boilerplate is really hard to get right in such a general manner. It allows VTK modules to consume non-VTK-module-driven projects, be consumed by the same, etc. A few examples I'd like to see:
|
Thanks for looking over this.
Yes, that would be nice. Upper case was the convention for the CMake build system for Trilinos in 2007 (started by Tim Shead) that TriBITS was created form and it stuck. Do you know of anyone who has written a tool that will lower-case the function, macro and command names and calls in CMake files? If one does not exist, would someone at Kitware like to write one? We have some funding for work like this. Then every project can use that tool to properly lower-case their function, macro, and command names and calls.
It is actually really handy to allow arbitrary CMake code in these files if used carefully (I can shown examples even in Trilinos). Since 95% of these files just call a single macro
Currently in TriBITS, the executables in a package are automatically linked to the libraries in that package. That is what happens 99% of the time in Trilinos, for example, so it made sense to make that the default to avoid boiler plate and mistakes. (That may lead to overlinking in some cases but that is rare.) Once TriBITS is refactored to use modern CMake for all targets (#299), this can be relaxed and allow optionally passing in the list of libraries to tribits_add_executable() (i.e. by adding an optional
That was needed for the Windows build to work at one point years ago. It may not be needed with current versions of CMake and Windows, who knows. Once we get a GitHub Actions Windows build added (as part of #363) we can revisit this.
It is not pointing to an install of TriBITS. Not sure how that model would apply to TriBITS in its source tree given the find rules of
You can't link a shared lib to an upstream static lib unless the static lib is built with
That is the case with the current TriBITS but after refactoring to use modern target-based CMake, more flexibility will be allowed as per #342 (comment). The key to enabling this is adopting this proposed standard. If all CMake code adopts that standard, then all of this should work seamlessly. But in general, trying to create big CMake projects out of CMakeLists.txt files that are developed by different teams using raw CMake is very risky and error prone unless everyone uses CMake in a very consistent way (at least that has been our past experience). Perhaps modern CMake will improve this situation but all it takes is one sloppy usage of At some point, you have to give up and just wrap non-compliant CMake code in their own CMake project build and wrap them in a larger CMake project (like a super build with CMake ExternalProject does). That can be done with TriBITS even right now as demonstrated in the WrapExternal package. By being so being so heavy handed with CMake, TriBITS has allowed single CMake projects with upwards of 500+ packages and subpackages and dozens of repositories by different teams to be integrated in to single CMake builds with one huge ninja dependency file. That has a lot of advantages. But we also want to support breaking these CMake projects into smaller CMake projects with more flexibility. That is the goal of #367.
Can you elaborate on this use case?
You mean a project using raw CMake pulls in a pre-installed piece of software that uses TriBITS; like a down-stream raw CMake project that uses Trilinos? If that is the case, then sure, that happens all the time with Trilinos with the example shown at:
Are any projects using VTK Modules other than the VTK project itself? Are there any simple examples of projects using VTK Modules? |
No, but this should at least be a 90% solution (with some
It does leave some stray whitespace around (namely It will only affect calls which are already all uppercase (ignoring things like
It's not so much bugs that I'm worried about but things like: if (obscure_config_condition)
list(APPEND deps featuredep)
endif ()
tribits_package_define_dependenices(${deps}) Figuring out why
OK, makes sense. VTK modules are, conceptually, each a single target, so that's where that difference lies.
Agreed. I've found that using targets for all of this is way better. For project-wide flags, it's better to make a target that has the
Yes. If I'm using TriBITS, do I need to do something to wrap every dependency I use or can I just use packages which provide CMake targets?
https://gitlab.kitware.com/vtk/vtk/-/blob/master/Examples/Modules/UsingVTK/CMakeLists.txt For a more complicated project that doesn't use VTK for its main bits, but uses VTK's module system for the VTK-interacting bits: |
CC: @keitat
It would be nice to also change the name of the function or macro being defined as well so if you have:
you would like it to be:
I think a carefully written Python script could do this and preserve the desired whitespace. But if you do this, this tool would need to be run on any branches that you might want to merge in or you will get major merge conflicts. (So the tool would need to be complete and very robust so its could be trusted to do everything correctly.)
Right, but if you don't allow that, then people will just take to generating the dependency files if they need this flexibility (thereby defeating the purpose of removing this flexibility). With TriBITS, you can dump the dependencies at configure time so it is easy to see what dependencies are actually being defined if there is confusion. But this type of thing is very rare. But when it is needed, it is very useful.
But targets don't not change anything for how tests get run with MPI or CUDA or threads or avoid name conflicts for test names. You need a strong convention for how such tests are named, defined, and run. I don't trust software at all that builds if I don't see tests running and passing on the target platforms. It is possible to do that using just test support functions like
With refactored TriBITS (#299), if the CMake software you are pulling in with
No, I mean a simple CMake project that uses the VTK Module system but does not use VTK at all. Something small and fairly simple like: but where TriBITS is stripped out and VTK Modules is used to control internal dependencies and what CMakeListx.txt files actually get processed in order. Does something like that exist? My plan for #63 and #299 is to take the existing
That will allow developers a clear apples-to-apples comparison of the different options and how well they scale (but this is still a really small example). |
@mathstuf, another point of reference on this but note the Spack allows the specification of dependencies to depend on runtime information from within the package's |
I'm aware and that is…vaguely possible (the file would need to be generated in the source tree or the source tree copied to the binary tree for this to actually work). But at that point, someone should be asking themselves "why is this so difficult?" and discover the rationale. The main purpose of doing it in VTK is to not have the public API depend on various CMake configuration options. This makes it very hard for consumers of a module to know whether some API is available or not. It is far better to instead make a module that itself appears or disappears based on the flags. This is because
Well, the module system ships with VTK, so that's kind of difficult. But nothing stops that example project from not using any |
This Issue is to capture things we can learn from the recent upgrade of the Kitware VTK Module system to use modern CMake.
The text was updated successfully, but these errors were encountered: