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

Install geosop using option BUILD_GEOSOP #445

Closed
wants to merge 1 commit into from

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented May 3, 2021

I was expecting to find this new tool in bin, but this PR should fix that.

@mwtoews
Copy link
Contributor Author

mwtoews commented May 3, 2021

A second commit here adds an option to not build/install geosop, which might be wanted by folks that only need the library. E.g.:

cmake -DBUILD_GEOSOP=OFF ..

will disable the build/install of geosop to /path/to/bin. The default for this option is ON.

Also note that I've verified that make uninstall works as expected, with this option configured either ON or OFF.

@pramsey pramsey requested a review from dr-jts May 3, 2021 17:22
@@ -9,4 +9,8 @@
# See the COPYING file for more information.
################################################################################

add_subdirectory(geosop)
option(BUILD_GEOSOP "Build geosop command-line interface" ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about calling this GEOS_BUILD_GEOSOP, so that GEOS options are grouped together?

(And we should really change a couple of the others, like DISABLE_OVERLAYNG ==> GEOS_DISABLE_OVERLAYNG)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is also an existing pattern for BUILD_.... So could continue with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I know what you mean and did briefly consider the two conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it helps, PROJ's CMake options use a BUILD_NAME convention for each tool, which is what swayed my decision here. The main reason why GEOS_ is prefixed to many CMake variables is to avoid clashes with other variables when creating large CMake projects. No other tool is named geosop, so no clash.

@mwtoews mwtoews mentioned this pull request May 24, 2021
@mwtoews mwtoews changed the title Install geosop runtime to CMAKE_INSTALL_BINDIR Install geosop using option BUILD_GEOSOP Jul 5, 2021
@mwtoews
Copy link
Contributor Author

mwtoews commented Jul 5, 2021

I've merged the previous commits and made a few subtle changes, including to show a message for this option, and to remove unnecessary "RUNTIME" from the install command, which should make CMP0006 NEW policy behaviour happier. (An alternative would be to specify both "RUNTIME DESTINATION" and "BUNDLE DESTINATION").

It seems there are a few unrelated CI failures.

@pramsey
Copy link
Member

pramsey commented Sep 30, 2021

Thanks! Merged.

@pramsey pramsey closed this Sep 30, 2021
@mwtoews mwtoews deleted the geosop-install branch November 12, 2021 09:15
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