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

Report versions of gm, ffmpeg, etc. #1851

Closed
wants to merge 4 commits into from
Closed

Report versions of gm, ffmpeg, etc. #1851

wants to merge 4 commits into from

Conversation

PaulWessel
Copy link
Member

Finally figured out how to deal with programs such as gm and ffmpeg who decided it is a good idea not to have a --version option, so we instead are forced to grep and awk and cut etc to get the obvious answers.

Finally figured out how to deal with programs who decided it is a good idea not to have a --version option so we instead can grep and awk and cut etc to get the obvious answer.
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

What if we don't have bash on Windows?

@PaulWessel
Copy link
Member Author

OK, shall I put it inside
if (WIN32)
and only do the version if not Windows?

@seisman
Copy link
Member

seisman commented Oct 20, 2019

In the top CMakLists, we have the FindUnixCommands directive, which can find if bash, gzip, tar et al. are available (see https://cmake.org/cmake/help/latest/module/FindUnixCommands.html).

include (FindUnixCommands)

I believe we can check if ${BASH} is defined.

@seisman
Copy link
Member

seisman commented Oct 20, 2019

The Windows CI has bash, and returns:

*  Found GhostScript (gs)     : yes
*  Found GraphicsMagick (gm)  : no
*  Found ffmpeg               : no
*  Found open                 : yes
*  Found ogr2ogr              : yes (2.4.1)
*  Found gdal_translate       : yes (2.4.1)

@PaulWessel
Copy link
Member Author

So no gm and ffmpeg installed? The test checks if they exist before we try to get versions. And seems to work for the GDAL tools.

@seisman
Copy link
Member

seisman commented Oct 20, 2019

gm and ffmpeg are only installed when we run the whole tests.

@PaulWessel
Copy link
Member Author

OK, so it works on Windows if bash is installed. is it likely that there will be people running cmake on Windows and not have bash? 99.9% of Windows users will install from the binary installer, no? Is it worth adding an if-test? Perhaps cmake will stop if one of these fails due to lack of bash, so safest to add that protection?

@seisman
Copy link
Member

seisman commented Oct 20, 2019

This is what I get on Windows. I have git bash installed, but I don't add it to my PATH, so it's invisible to cmake.
image

OK, so it works on Windows if bash is installed.

Strictly speaking, no. Having bash installed doesn't mean tr and cut are also available. But I think it's OK to me. If someone doesn't have bash, tr or cut, then the versions are empty.

src/CMakeLists.txt Outdated Show resolved Hide resolved
@PaulWessel
Copy link
Member Author

Can we do better than the awkward tr ',' ' ' | cut -f2 -d' and head -n 1 | cut -f2 -d' ' ?

Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

OK for me. Hopefully it also works for @joa-quim.

@PaulWessel
Copy link
Member Author

Let's wait to accept this PR until @joa-quim has tried it.

One less pipe and adds GDAL in front.
@seisman
Copy link
Member

seisman commented Oct 20, 2019

Can we do better than the awkward tr ',' ' ' | cut -f2 -d' and head -n 1 | cut -f2 -d' ' ?

I believe I found a better way. Will try if it works.

@PaulWessel
Copy link
Member Author

I guess it is a question of ${AWK} is more likely to be available in Windows than tr and head. We could do

gm help | awk '{if (NR == 1) print $2}'
ogr2ogr --version | awk -F, '{print $1}'

for instance.

@seisman
Copy link
Member

seisman commented Oct 20, 2019

Closed as #1853 is better.

@seisman seisman closed this Oct 20, 2019
@seisman seisman deleted the reportversions branch October 20, 2019 05:04
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

2 participants