The motivation for this change is to allow applications to link multiple major versions of VTK into the same application.
A simple example of this could be an application A depends on library B and VTK 9, and library B depends on VTK 8. At runtime, both VTK 9 and VTK 8 will need to be loaded to satisfy the dependencies for both application A and library B. The problem with this is the symbols from VTK 9 will conflict with the symbols for VTK 8.
Objective
The goal of this change is NOT to support applications building against multiple version of VTK in the same project. The goal is to only support linking external project(s) that support a different version(s) of VTK than the main application/each other.
Proposal
The current proposal of how this implementation would work is as follows.
Add a CMake option for naming the ABI namespace VTK_ABI_NAMESPACE_NAME. Default is vtk${VTK_MAJOR_VERSION}
That is not exactly the same thing. That is adding namespaces to the wrapping code so that if you have something in a namespace that shows up in a namespace in python. In this case with an inline namespace, we don’t want to change the resulting python wrapped code at all. We just need the parser to ignore the inline namespace when doing the wrapping. @dgobbi what is the best way to do that?
All that’s needed is a rule in vtkParse.y to ignore the “inline namespace” declaration, it’s a one-line change. But vtkParse.tab.c will also need to be regenerated. I’ll put together an MR later today.
By only defining the namespace macro to expand when __VTK_WRAP__ is not defined seems to work as expected without any changes to the vtkParser. The current problem with this implementation is not all files include vtkABI.h which is where I have the macros defined at the moment.
Edit: Scratch that, it still gets stuck on the macro.
If adding something to ignore inline namespace is not too hard though, I think that would be preferred.
This is best because any VTK-using project wanting to do the same thing will have its own inline namespace; the literal names used here will not be usable generally.
The default is to not have a namespace at all, correct?
because otherwise a build tree that configures with VTK9 but later checks out VTK10 will continue to be mangled with vtk9. This is not good, so the above is preferred. However, with this behavior, switching between branches is now a rebuild of the entire tree because the namespace changed. In that sense, I think the core default should be an empty namespace.
So I ran into an issue with forward declarations that will cause issues with existing VTK codes after adding the namespace.
In a lot of VTK code there are forward declarations of classes…
class vtkObject;
To the compiler, this creates a new declaration of vtkObject that is not namespaced and becomes ambiguous between ::vtk9::vtkObject and ::vtkObject. Right now the quick fix in the examples is to just add the VTK_ABI_NAMESPACE_{BEGIN,END} macro around the forward declarations.
VTK_ABI_NAMESPACE_BEGIN
class vtkObject;
VTK_ABI_NAMESPACE_END
This is rather verbose and ideally the downstream user shouldn’t have to think about these macros.
Two possible solutions to this problem immediately come to mind. This first solution is to just add another layer of abstraction, similar to do what I did for the AutoInit DECLARE macro, and hide the namespace inside of a macro.
vtkForwardDeclare(class vtkObject);
Another possible option is to inject forward declarations of VTK module classes into the <ModuleName>Module.h headers or a <ModuleName>Fwd.h header that just forward declares everything in the module. In that case, users would remove all of the VTK forward declarations from their source and just include headers.
@ben.boeckel@bill-hoffman Do you guys, or anyone else, have better ideas on how to solve this or a preference/strong objection to either/both of the ideas here? Personally I think the former (macro) is ugly and verbose and the latter (forward header) is a more C+±style solution. But both are going to require changes by the downstream user.
I agree that the former is ugly. The latter requires some tooling to ensure that it matches up with the set of public classes (basically just more HeaderTest work). Without some way that shows up in CI that flags when the Fwd header is out of sync, I don’t think the latter is any better in the long run.
Have you seen any major library that isolates different library versions using namespaces? If yes, we may get good ideas how they solved the problems you have run into. If there are no (or very few examples) then using namespaces for this may not be a good idea.
We have come across this problem when building applications that worked with external hardware that came with SDKs that used VTK and Qt internally. We did not want the SDK to dictate what VTK+Qt versions our application should use, so we asked the SDK developers to fix this. They changed their build system to build VTK statically and mangled Qt DLL names and it was all good. There was no need to change VTK or our application in any way. A similar example is how an application can use many different C runtime versions - there is no messing with namespaces there either.
If you cannot ask the third-party developers to change how they build VTK then you may be able to use manifests or rpaths to ensure each executable loads the correct VTK version.
Not via a per-version mechanism, no. Qt and Boost allow one to mangle using some pre-build process, but that’s about as far as I’ve seen C++ libraries go. It’s basically what VTK is doing.
This works on Windows and macOS where symbols are looked up in a per-library namespace. It doesn’t work at all on Linux where all public symbols get dumped into a single lookup table. One could make the VTK symbols non-exported, but that isn’t something we support either.
This is due to the symbol lookup mechanisms; it is a platform feature that isn’t everywhere.
Again, this leaves Linux out to dry as the loader will see duplicate sonames and just say “nope, already did that” before driving right off a cliff.
It seems that the motivation for the proposed change is not very strong:
no other libraries are doing this (if I understood your comment correctly)
VTK has been fine without this mechanism until now (for almost 30 years and thousands of VTK-based projects)
there are not-too-complicated alternative solutions for Windows and macOS
you can find solutions for Linux, too (e.g., work with the third-party library developer to support more recent VTK versions)
If you wanted to do a small, non-invasive change then even a weak motivation can be sufficient.
However, if you want to introduce a new special VTK mechanism (such as VTK_ABI_NAMESPACE_BEGIN or vtkForwardDeclare) that impacts every class in VTK proper (and potentially all VTK-based projects) then you would need to demonstrate high value for the entire VTK community. Apart from solving VTK upgrade problem in your project, would there be other useful applications of this new namespace mechanism?
That’s the push for this effort. The other project just cannot update (because VTK still keeps breaking API despite my efforts to try and push back on such things).
ParaView 100% doesn’t care about this, so I don’t think it should care at all. It should just be limited to VTK (that is, every project wanting to do this can make their own decision about it and how to do it). Note that VTK also doesn’t provide mechanisms intended to work for anything not VTK either (that is, there is no exported framework/API for doing this to an arbitrary project).
Yes, there are a couple of libraries in Boost, notably Boost.Log and Boost.Filesystem, that utilize this mechanism for versioning. That said, that is not what this change is for, it was just convenient to use the major version as a default example. It seems both Qt and Boost also provide tools for mangling all symbols .
Just a note, the proposed changes here are 100% opt-in for the foreseeable future. The purpose is the make handling VTK updates easier in the cases where up/down-stream projects also depend on different major versions of VTK.
While it does touch most of the files in the main VTK repo, it is non-intrusive to all downstream code except in the case of forward declarations. So what that means for VTK-based projects is if they want to use a mangled VTK, they will need to do something different to handle the forward declarations. Otherwise it is business as usual.
Thanks, it was a good pointer. I remembered that Qt supports custom infix for DLL names, but not that it also supports placing Qt into a custom namespace - see Qt in namespace. It makes the proposed change much more acceptable, because:
It is much easier to justify introduction of this complex mechanism if we can show that another well-known libraries do the same.
There is a proven implementation that we can learn from: the risks of introducing unforeseen complications is much lower and there is no need to invent new things (for example, I would use the same macros that Qt uses for forward declarations).
Qt implementation has been proven to not affect external Qt code (if Qt is built without namespace then external code does not need to bother with the namespace macros).
There could still be some VTK-specific issues, for example how to handle symbols in third-party libraries (can they be placed in the namespace?). It is also hard to imagine that it is really impossible to hide internal use of an old VTK version with a static build and hiding/stripping of private symbols (which would be a much less invasive change).
Yes, this is still a problem that needs tackled with this approach.
Yes, but the (my) confidence in any such solution actually working in a production-ready way is also near the “good luck, have fun storming the castle” level. And the crashes from these kinds of things are not typically obvious or easy to reason about.