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 CMake option to disable package export #2285

Merged
merged 2 commits into from Aug 14, 2016
Merged

add CMake option to disable package export #2285

merged 2 commits into from Aug 14, 2016

Conversation

gentryx
Copy link
Member

@gentryx gentryx commented Aug 11, 2016

My autobuilds of LibGeoDecomp with HPX have been randomly crashing in the past weeks. They're running on multiple machines and some of these machines have a shared NFS. If one machine is running CMake for LibGeoDecomp and another machine is simultaneously deleting an HPX build tree, then CMake may fail because of some missing files. This patch enables users to switch off package registration.

It's also good practice not to clutter a user's home directory/registry and bar the user from disabling this.

…itions if a user is doing multiple builds of HPX and dependent concurrently
@sithhell
Copy link
Member

I'm actually not sure what to respond here ... what you describe is clearly CMake misbehaving. Exporting packages is something CMake advocates. Could you please elaborate on the effect of not exporting the package? How does it affect linked libraries, include directories and compiler flags?

@gentryx
Copy link
Member Author

gentryx commented Aug 11, 2016

The only purpose of export(PACKAGE) in CMake is to register a package in the user's package registry (see https://cmake.org/cmake/help/v3.0/command/export.html ). On Windows this is the Registry, on Unix variants it's a file below ~/.cmake/PACKAGES.

Not exporting a package doesn't affect linked libraries, include directories, compiler flags or anything else. The only effect is that a user might have to specify -DCMAKE_PREFIX_PATH so that a dependent code can find HPX via CMake.

CMake's config files are not resistant to such race conditions. We can't fix that. With this patch we'd give HPX users a way to work around this issue. Please keep in mind that my patch preserves the current behavior as the default. It's purely optional to disable package registration (and would help me a lot).

@gentryx
Copy link
Member Author

gentryx commented Aug 12, 2016

Ping

@hkaiser
Copy link
Member

hkaiser commented Aug 12, 2016

LGTM, thanks!

@sithhell
Copy link
Member

sithhell commented Aug 12, 2016

IIUC, this is to circumvent a race in the filesystem, right?
Just for my understanding: you have independent builds, all of which work
with different build trees and you see failures due to concurrent
creation/deletion of those trees?
That sounds a little absurd. Do the cmake guys know about this?

@gentryx
Copy link
Member Author

gentryx commented Aug 13, 2016

Yeah, I have multiple independent builds in separate directories (e.g. ~/test_build_with_options_xyz/{hpx/libgeodecomp}/ and ~/test_build_with_options_abc/{hpx/libgeodecomp}). Let's say build xyz is about to end while build abc is just now running CMake. Because build xyz registered its HPX build via export(PACKAGE), it is also visible to abc. CMake is then parsing ~/test_build_with_options_xyz/hpx/build/lib/cmake/HPX/HPXConfig.cmake. Meanwhile build abc is deleting its build tree. CMake may then fail because HPXConfig.cmake is calling include() for files which have just vanished.

Bottom line is: we would not see this if

a) CMake would ignore package configs that are incomplete, or
b) the HPX config was in a monolithic file (though that would still cause other troubles later), or
c) if we could avoid making HPX builds visible to each other.

In this PR I implemented option c.

@sithhell
Copy link
Member

Thanks, guess that's fine now! This will have the effect that HPX isn't magically found anymore, which led to a lot of problems in the past.

@hkaiser
Copy link
Member

hkaiser commented Aug 14, 2016

I like that!

@hkaiser hkaiser merged commit 1b7acf0 into STEllAR-GROUP:master Aug 14, 2016
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