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

STIR registries files are not installed (STIR_REGISTRIES points to original sources) #211

Closed
KrisThielemans opened this issue Jul 3, 2018 · 8 comments · Fixed by #398
Closed
Labels
Milestone

Comments

@KrisThielemans
Copy link
Collaborator

STIR_REGISTRIES is a CMake variable that lists all .cxx files that add items to registries, e.g. for IO, see the Registries of classes section in the STIR developers guide.

At present, we do not install these files. However, anyone who uses an installed version of STIR will need them for IO, filtering etc to work. Therefore, at present, an installed version of STIR will only work on the original developer's machine (if she doesn't delete the source files).

Installing source files is "not done" however. Where would we put them?

If we'd install them, we'll have to adjust the variable in STIRConfig.cmake.in

@KrisThielemans KrisThielemans added this to the v4.0 milestone Jul 3, 2018
@KrisThielemans
Copy link
Collaborator Author

some extra info.

The registries contain static variables that have as side-effect that they add a class to a particular registry (for factories such as for IO).
These variables need to remain alive as long as STIR functionality is used (their destructor removes the class from the registry). The STIR developer's guide attempts to explain this more.

The registry files need to be compiled and explicitly linked. If we'd stick them in the library, most linkers will actually never include them in the linked executable/library as the registries don't contain code that is being called from elsewhere. Hence the STIR_REGISTRIES variable (see #83 for an alternative way using object libraries).

@KrisThielemans
Copy link
Collaborator Author

Short-term solution is to install the files and adjust the STIRConfig.cmake.in. Longer-term solutions can be discussed in #212.

@KrisThielemans
Copy link
Collaborator Author

This bug affects the conda script conda-forge/staged-recipes#5460

@ashgillman
Copy link
Collaborator

Ah, I misunderstood previously. They aren't required at runtime, rather at compile time for dependent libraries (e.g., SIRF)? Makes sense

Still, why are the .cxx files required? More common would be a header file (.h) and a compiled implementation (.so)?

Re: install location:

  • If .so will suffice, it would likely go with other libs in ${CMAKE_INSTALL_PREFIX}/lib
  • If we should also include a header, it could go with the others in ${CMAKE_INSTALL_PREFIX}/include/stir
  • If the .cxx is needed, perhaps ${CMAKE_INSTALL_PREFIX}/share/stir is appropriate? Or possibly install to same location as headers?

@KrisThielemans
Copy link
Collaborator Author

The registry files need to be compiled and explicitly linked. If we'd stick them in the library, most linkers will actually never include them in the linked executable/library as the registries don't contain code that is being called from elsewhere.

Here's an old post where I asked about this.

It might be that a .so would solve it, but

@ashgillman
Copy link
Collaborator

Since it is Friday, perhaps a visual response is acceptable.

image

@ashgillman
Copy link
Collaborator

Do you know if the entire source needs to be present for other projects, or just the registration sources?

I'd suppose easiest solution is to copy the source (all or just registries?) to ${CMAKE_INSTALL_PREFIX}/share/stir, but you are far more familiar with the code base than me. Is this sort of issue being tackled at the hackathon?

@KrisThielemans
Copy link
Collaborator Author

only the registration sources. let's not install anything else. I agree that ${CMAKE_INSTALL_PREFIX}/share/stir is a good place. I guess it makes most sense to just preserver directory structure.

As people using the ${STIR_LOCAL}/extra_dirs.cmake might have registries themselves (I do), it might be best to add the INSTALL targets in a loop over all files in STIR_REGISTRIES.

the somewhat tricky bit is to adapt the STIR_REGISTRIES in the STIRConfig.cmake.in. Might need some generator expressions.

not a trivial exercise...

It isn't really on the list of things for the hackathon, but we can certainly use it to solve some of those "small" issues with a few people.

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

Successfully merging a pull request may close this issue.

2 participants