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

Fix installed header directory structure #2754

Merged
merged 5 commits into from
Nov 11, 2019

Conversation

ricrogz
Copy link
Contributor

@ricrogz ricrogz commented Oct 31, 2019

Installing RDKit breaks the directory structure for headers: directories under Graphmol/ChemReactions/ and Graphmol/MolStandardize/ get flattened.

So if you include, e.g. Graphmol/ChemReactions/Enumerate.h, it includes EnumerateBase.h, which in turn includes "../Reaction.h", which in the source is in the parent directory but in the installation is in the same directory as Enumerate.h and EnumerateBase.h, instead of in the parent directory as specified, thus breaking the build.

I patched the CMakeFiles to reproduce the same directory structure as in the sources.

Also, some header files that get included from other header files are not included in the installation, e.g. EnumerateTypes.h or Target.h. I added those to the installation.

Finally, path "hash" under RDGeneral gets duplicated in the installation, effectively placing its content at ${installed_include_path}/RDGeneral/hash/hash/*. When I patched this, I removed the exclusion of ".svn" as I couldn't find it anymore.

Copy link
Member

@greglandrum greglandrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one change about shrinking the number of headers for FMCS instead of expanding it

Code/GraphMol/FMCS/CMakeLists.txt Outdated Show resolved Hide resolved
@d-b-w
Copy link
Contributor

d-b-w commented Nov 7, 2019

This looks good to me. I do have one question about future maintenance, though. It seems like it would be easy to forget to add a new header to the installer, or to put it in the wrong place. Is there a way to make sure that we don't misplace any required headers?

Would it be possible to use a glob and a set of exclusions? (We could spin that off as a separate issue, but I just wanted to raise the concern while we're thinking about this).

@ricrogz
Copy link
Contributor Author

ricrogz commented Nov 8, 2019

Would it be possible to use a glob and a set of exclusions? (We could spin that off as a separate issue, but I just wanted to raise the concern while we're thinking about this).

Given that I removed some headers from the FMCS CMake file, I would say that using globs all over would end up installing more headers than those that we want to be exported.

Rather than that, I would extract the "public" headers to a separate include dir, and have that one installed. It would certainly take some work to implement, but from that point on, any changes in the include dir would automatically reflect in the installation.

@d-b-w
Copy link
Contributor

d-b-w commented Nov 8, 2019

Good point. Let's think about that next time one of us wants to add a new header.

@greglandrum greglandrum merged commit 7bb564b into rdkit:master Nov 11, 2019
@greglandrum greglandrum added this to the 2020_03_1 milestone Nov 11, 2019
@ricrogz ricrogz deleted the fix_installed_header_dir_structure branch November 11, 2019 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants