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

Find or build maeparser & coordgen libraries #2064

Merged
merged 7 commits into from Nov 12, 2019
Merged

Find or build maeparser & coordgen libraries #2064

merged 7 commits into from Nov 12, 2019

Conversation

ricrogz
Copy link
Contributor

@ricrogz ricrogz commented Oct 21, 2019

This is meant as an improvement: with these changes, CMake first checks if there is a build of Schrödinger's maeparser and coordgen libraries that can be used in openbabel's build. If there isn't, then, this patch downloads the sources and builds them.

This means that it addresses #2015.

Some notes:

  • CMake will, by default, look in standard library and include directories for the built library and the required headers. Other directories can be specified for the lookup by using "maeparser_DIR" and "coordgen_DIR".
  • By default, the "master" branch of maeparser and coordgen will be downloaded from GitHub. This can be changed using the "MAEPARSER_VERSION" and "COORDGEN_VERSION" variables, pointing them to specific tags, branches or commits.
  • If sources are already present at the directory where CMake would put them, the download step will be skipped, and the available sources will be used.
  • Both maeparser and coordgen test executables will not be built. In case they are, ctest won't be able to find them, since they will be put in the "bin" directory, together with the rest of the openbabel executables.
  • Boost threads need to be disabled in case of a static build, or the build will fail (I haven't investigated this too much, but probably the Boost libraries are not built using --static-libgcc.

@ghutchis
Copy link
Member

This seems a bit premature since we don't use coordgen yet. ;-)

But the approach is good. If it's okay with you, I'm going to hold off on review until that time?

If you want to revise to only do the maeparser bits now, I'll review sooner.

@ricrogz
Copy link
Contributor Author

ricrogz commented Oct 24, 2019

I included the coordgen part because I thought it might be helpful for implementing it and trying it out. Also, I wanted to plan a bit ahead, to check that maeparser finding/building was incorporated in a way that was compatible with adding in coordgen.

I'm ok with both options, although I think it might be best if I removed the coordgen part for the time being, and the maeparser one was merged, even if just to update it to a more recent version.

Maybe @lorton has a preference about this?

@lorton
Copy link
Contributor

lorton commented Oct 24, 2019

@ghutchis - I don't think there's a rush here, but once coordgen is built/linked I'll be able to proceed with the case in adding the ability to optionally use coordgen to generate coordinates. We're in a bit of a chicken and egg because I'm hesitant to try to contribute code to use coordgen until it's building cleanly in openbabel, and openbabel doesn't have priority to build cleanly until coordgen is being used ;)

CMakeLists.txt Outdated

else()

set(MAEPARSER_VERSION "master")
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit hesitant at that. I don't mind "let's use master for OB master".. is the API for maeparser fixed? After all, if maeparser changes, does the code here in OB need an update too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That can be changed, of course. In fact, I just noticed a couple of things I forgot about when I wrote that. I'll update shortly.

CMakeLists.txt Outdated

else()

set(COORDGEN_VERSION "master")
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@ricrogz
Copy link
Contributor Author

ricrogz commented Nov 11, 2019

In this update:

  • Swapped the default "master" versions with the last released versions of maeparser and coordgen.
  • Made the version strings cached, so that the versions can be overriden from the cmake compile line (-DMAEPARSER_VERSION=...).
  • GitHub does one kind of dumb thing with the releases: while non-release tags like "master" are unpacked into directories like "[project]-[tag]" (e.g. "maeparser-master"), the release tags are named like "v[x].[y].[z]", but when they are unpacked, the directories are called "project-[x].[y].[z]" (dropping the "v"), so we need to account for that when in the path to the sources.

@ricrogz
Copy link
Contributor Author

ricrogz commented Nov 11, 2019

Oh, I see why I used maeparser's master: v1.2.2 (the latest) did not yet include the flag to disable the unit tests... Other than that, it is fine.

@ghutchis
Copy link
Member

@ricrogz - I can't merge something that fails tests. If master is needed for that, no problem. I just want to make sure that we can freeze versions for e.g openbabel-3.1.0

I'm aware of the annoying GitHub tar/zip variance. Happy to work on that.

@ricrogz
Copy link
Contributor Author

ricrogz commented Nov 12, 2019

@ricrogz - I can't merge something that fails tests. If master is needed for that, no problem. I just want to make sure that we can freeze versions for e.g openbabel-3.1.0

Yeah, sorry, I forgot that the switch to disable the test was only in maeparser's master. I'll restore the "master" versions for now.

I didn't really answer your question: the APIs to maeparser and coordgen are rather stable (we don't make many changes that require updates to calling code, and I'm happy to assist when that happens), so it should be ok to keep it set to "master", and change it to whatever version is the current one when you prepare for a release.

I'm aware of the annoying GitHub tar/zip variance. Happy to work on that.

I gave it a try already, using find_path to search for the directory in which maeparser/coordgen's CMakeLists.txt lives. That seemed to work fine in the CI builds.

@ghutchis ghutchis merged commit 8500f8a into openbabel:master Nov 12, 2019
@ricrogz ricrogz deleted the find_or_build_maeparser_coordgen branch November 14, 2019 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants