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

Drop find_programs gs #728

Merged
merged 2 commits into from
May 17, 2019
Merged

Drop find_programs gs #728

merged 2 commits into from
May 17, 2019

Conversation

seisman
Copy link
Member

@seisman seisman commented May 17, 2019

gswin32 is another possible name of ghostscript.

@seisman seisman requested a review from a team May 17, 2019 14:00
CMakeLists.txt Outdated
@@ -72,7 +72,7 @@ set (CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/modules/"
# Find UNIX commands
include (FindUnixCommands)
find_package (Git)
find_program (GS gs gswin64)
find_program (GS NAMES gs gswin64 gswin32)
Copy link
Member

Choose a reason for hiding this comment

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

Actually I think what is used on Windows are the gswin64c/gswin32c but I don't know why do we search for the ghost on Windows. The only module that uses it is psconvert, and it finds the executable via a registry inquire, which was a complicated thing to achieve because Windows64 have two registries one for 32 and another 64 and the Ghost will be in either one or the other depending on its bitage.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. Seems we even don't need to search gs on Linux. The variable GS is never used in the cmake configuration or the psconvert source code. Maybe we can remove the find_program (GS ...) line?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, it looks we can drop it.

@seisman seisman changed the title Find gswin32 Drop find_programs gs May 17, 2019
@seisman seisman requested a review from joa-quim May 17, 2019 14:54
Copy link
Member

@joa-quim joa-quim left a comment

Choose a reason for hiding this comment

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

Tried on Win and apparently nothing breaks, so good to go for me.

@seisman seisman merged commit c1128d2 into master May 17, 2019
@seisman seisman deleted the fix-gswin32 branch May 17, 2019 15:59
@PaulWessel
Copy link
Member

Question: Since GMT requires gs to be present, especially modern mode, I think finding gs was our proxy for making sure the user has gs installed. Will this go away now? I.e., users can build GMT if they do not have gs installed?

@seisman
Copy link
Member Author

seisman commented May 17, 2019

find_program (GS gs) tries to find gs and save its full path to the variable GS. However, if gs isn't installed or isn't found, find_program doesn't give any warnings or errors.

@joa-quim
Copy link
Member

joa-quim commented May 17, 2019

But I doubt it ever detected the Ghost presence on Windows that does not install itself in PATH. This is what we do in psconvert

#ifdef WIN32
	if (ghostbuster(GMT->parent, C) != GMT_NOERROR)  /* Try first to find the gspath from registry */
		C->G.file = strdup ("gswin64c");     /* Fall back to this default and expect a miracle */
#else

@seisman
Copy link
Member Author

seisman commented May 17, 2019

If you want to make sure gs is installed, we need something like

find_program (GS NAMES gs gswin64c gswin32c)
if (NOT GS)
message (..)
endif (NOT GS)

@PaulWessel
Copy link
Member

Well, I think we need to; it is not an optional install in my mind since psconvert calls it, and movie calls psconvert indirectly per frame via gmt end.

@seisman
Copy link
Member Author

seisman commented May 18, 2019

Strictly speaking, gs is a runtime dependency but not necessary when building GMT. I agree we need to check if gs is available.

@PaulWessel
Copy link
Member

Yes, and I dont know what the standard protocol is for tools that depends on executables. There must be other packages that depends on executables instead of includes and libraries. What do they do?

@seisman
Copy link
Member Author

seisman commented May 18, 2019

I don't know if there are any standard way to do that. But we have a FindSphinx.cmake in cmake/modules. And this file (https://github.com/Kitware/CMake/blob/master/Modules/FindCygwin.cmake) shows that we can search gs from registry.

@seisman
Copy link
Member Author

seisman commented May 18, 2019

This PR is already merged and there is no way to reopen it. I've opened an issue #733 in order not to forget it.

@PaulWessel
Copy link
Member

What you think, @joa-quim ? Is this something to pursue or is it just more work for no gain?

@joa-quim
Copy link
Member

Let's drop it. I see no real gain in detecting gs presence while building on Win

obaney pushed a commit to obaney/gmt that referenced this pull request Aug 18, 2021
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