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

Add Eigen to ExternalProject and automatically download if RDK_BUILD_DESCRIPTORS3D #3075

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

e-kwsm
Copy link
Contributor

@e-kwsm e-kwsm commented Apr 7, 2020

What does this implement/fix? Explain your changes.

Even if RDK_BUILD_DESCRIPTORS3D is ON, 3D descriptors calculators are not built if Eigen is not found.

This PR resolves the dependency.

Copy link
Contributor

@bp-kelley bp-kelley left a comment

Choose a reason for hiding this comment

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

I think this is a good idea, but it is hard to test as our current builds already have Eigen installed. Perhaps we should add a non-eigen installed entry to the build matrix?

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.

LGTM

@greglandrum
Copy link
Member

@bp-kelley @ptosco @ricrogz : This PR adds a hard dependency on eigen, but it does download a copy if eigen isn't already installed.

Do any of you see a problem with this?

@ptosco
Copy link
Contributor

ptosco commented Feb 17, 2021

I am fine with this. Also, isn't the dependency enforced only if RDK_BUILD_DESCRIPTORS3D is ON? I would be happy with the dependency in any case.

@ricrogz
Copy link
Contributor

ricrogz commented Feb 17, 2021

I'm also ok with this.

@greglandrum
Copy link
Member

Also, isn't the dependency enforced only if RDK_BUILD_DESCRIPTORS3D is ON? I would be happy with the dependency in any case.

Ah, right. Maybe we should just add the dependency globally so that eigen can be used in other bits of the code without concern/complication?

@ptosco
Copy link
Contributor

ptosco commented Feb 17, 2021

Ah, right. Maybe we should just add the dependency globally so that eigen can be used in other bits of the code without concern/complication?

The RDKitJS build currently sets RDK_BUILD_DESCRIPTORS3D=OFF, so I probably wouldn't do that.

@greglandrum
Copy link
Member

@ptosco : I'm fine with merging this as is.
What's your take?

Copy link
Contributor

@ptosco ptosco left a comment

Choose a reason for hiding this comment

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

Looks good, as the build scripts will look for Eigen3 first, and only in case it is not found and RDK_BUILD_DESCRIPTORS3D=ON it will download Eigen3 from GitLab, otherwise it will use what was previously found (e.g., in /usr/include or in the conda environment).

@greglandrum greglandrum added enhancement infrastructure build infrastructure and the like labels Sep 30, 2021
@greglandrum greglandrum added this to the 2021_09_1 milestone Sep 30, 2021
@greglandrum
Copy link
Member

Thanks for the contribution @e-kwsm!

@greglandrum greglandrum merged commit 3720537 into rdkit:master Sep 30, 2021
@e-kwsm e-kwsm deleted the Eigen branch September 30, 2021 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement infrastructure build infrastructure and the like
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants