search

swami / libinstpatch

Instrument file software library.
Other
20 stars 6 forks source link

Add getopt support for Windows #11

Closed jjceresa closed 5 years ago

jjceresa commented 5 years ago

This support adds arguments parsing useful for any console application example using libinstpatch library. Actually riff_dump is one of these executable example that use getopt.

derselbst commented 5 years ago

Have you tested this? I would be surprised if getopt is available as pkgconfig.

jjceresa commented 5 years ago

I would be surprised if getopt is available as pkgconfig.

getopt.h is available elsewhere on the net. This way it is very easy, to install this header, and write a getopt.pc file and put all on the compiling host machine. (The documentation should be completed to gives an exemple of getopt.pc).

`getopt.pc presence is a convenience that shows to the user (via CMake) that getopt support is need for these Windows targets. Also it helps to write other simple console application to get familiar with libinstpatch.

Have you tested this?

Yes, riff_dump executable works.

derselbst commented 5 years ago

getopt.h is available elsewhere on the net. This way it is very easy, to install this header, and write a getopt.pc file and put all on the compiling host machine.

But as it's only a single header file check_include_file would be a better choice rather than writing up a custom pkgconfig file, in my opinion.

jjceresa commented 5 years ago

check_include_file would be a better choice rather than writing up a custom pkgconfig file, in my opinion.

I was thinking of this nice choice, but using check_include_file i don't know yet how to instruct CMake about the directory where to search getopt.h.

derselbst commented 5 years ago

check_include_file only tries to compile a C file that #includes the requested header. Thus it looks up the default include search path of our compiler. You may also specify CMAKE_REQUIRED_INCLUDES as semicolon separated list for additional include search paths. See here for more.

Alternatively you may drop getopt completely and search for an equivalent function that glib may provide.

jjceresa commented 5 years ago

You may also specify CMAKE_REQUIRED_INCLUDES as semicolon separated list for additional include search paths.

Nice infos, thanks. This one is implemented in commit https://github.com/swami/libinstpatch/pull/11/commits/563b8f66fc3075e1f3c34cebefc36c402dda217d

jjceresa commented 5 years ago

Sry, but hardcoding paths is bad practice.

Your are right. Please, this path shouldn't be understood as hard coded but default getopt path.

Please, let me explain better the intend. When a variable is declared CACHE (i.e GETOPT_INCLUDE_PATH) it becomes not only persistent but also interactive. That means its value can be changed by the user (unless FORCE is used; see note below) . Path "d:\freews\include\getopt" is the value taken the first time cmake is invoked. For next invocation cmake will take cached value the user had changed during previous invocation (through the console or cmakegui).

You should instead set CMAKE_REQUIRED_INCLUDES when calling cmake to configure the sources

The interesting interactive behaviour described above is more obvious using cmakegui. When invoked, cmakegui display previous cached value like this.

GETOPT_INCLUDE_PATH (Path of getopt.h) = "path chosen during previous cmake invocation"

In the case of getopt support, GETOPT_INCLUDE_PATH name should seem less confusing for user then CMAKE_REQUIRED_INCLUDES.

Note: for cached variable ,when FORCE option is used, this prevent user changing the value. In this case the value displayed in cmake-gui will remain constant (i.e hardcoded).

derselbst commented 5 years ago

In the case of getopt support, GETOPT_INCLUDE_PATH name should seem less confusing for user then CMAKE_REQUIRED_INCLUDES.

Ok, sure. Yet, I don't like it. Why using drive D:\ as default? Why this path in \freesw\? This is so specific to your setup. Nevertheless, we can go with GETOPT_INCLUDE_PATH for now. However you should append it to CMAKE_REQUIRED_INCLUDES rather than overwriting it. This should do:

set (CMAKE_REQUIRED_INCLUDES ${CMAKE_REQUIRED_INCLUDES} ${GETOPT_INCLUDE_PATH})

In the long run I would vote to replace getopt with glib's option parser though.

jjceresa commented 5 years ago

Why this path in \freesw\? This is so specific to your setup.

You are right, this is not a valid default path and should be replaced rather by an empty string.

However you should append it to CMAKE_REQUIRED_INCLUDES rather than overwriting it. Good point.

In the long run I would vote to replace getopt with glib's option parser though.

I think too. This replacement will avoid these light getopt dependency. Waiting for this replacement, this PR allows immediate use of riff_dump example.