swami / libinstpatch

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

Cmake adaptation - adding definition file (.def) - renaming .pc file #5

Closed jjceresa closed 5 years ago

jjceresa commented 5 years ago

1)This add a definition file (.def) for Windows shared library target. 2)Rename pkg-config file (.pc) to libinstpatch.pc

jjceresa commented 5 years ago

The set of previous 3 commits https://github.com/swami/libinstpatch/pull/5/commits/e3e48227d43eb3f0a76c041e4f95c244d2bcb8b4 , https://github.com/swami/libinstpatch/pull/5/commits/f2cef12d50562f1e7003a15d6dc39547ea847be8 , https://github.com/swami/libinstpatch/pull/5/commits/135a8bbe4671b6e3674b1c846a7bbdfac322bcc6 address the following: Advantages: 1) Removing automatic generation of builtin.c.h files , through perl script glib-mkenums avoid perl executable dependency which isn't natively installed on Windows. This opens simplification in the process chain cmake + compiling libinstpath project on any OS platform. 2) This suppress masking important files which are part of sources directory file. Inconvenients: When adding new enum type in builtin_enums.h one need to report manually the corresponding function in built_in.c file. This is really simple to do and shouldn't be a problem when new enumerations are added slowly over time.

jjceresa commented 5 years ago

I am fine with this PR, if you agree you can merge it in master. Later i will continue on cmake-adaptation branch to propose fixing: 1)Change target naming from instpatch-1.0 to libinstpatch-X.Y with X.Y being major.minor version value. 2) same remark for installed sub-directory name libinstpatch-1.0. install ( FILES ${instpatch_public_HEADERS} ${public_main_HEADER} ${CMAKE_CURRENT_BINARY_DIR}/version.h DESTINATION include/libinstpatch-1.0/libinstpatch )

derselbst commented 5 years ago

You have already added builtin_enums.[h|c] in #1, which I haven't merged yet because point 4) seems to be not yet done. Do you consider #1 to be ready as well?

I disagree with removing the glib-mkenums command from cmake. This should be rewritten to a custom target, so that it's only executed when really needed, e.g. when explicitly issuing make enums. I can draft a proposal for this, if you want.

jjceresa commented 5 years ago

You have already added builtin_enums.[h|c] in #1, which I haven't merged yet because point 4) seems to be not yet done.

In PR#1, builtin_enums.[h|c] have already added in source directory only, but not in CMakeLists.c. I will do a commit for completeness. I think point (4) is done. I will confirm that in PR#1 conversation.

I disagree with removing the glib-mkenums command from cmake.This should be rewritten to a custom target, so that it's only executed when really needed, , e.g. when explicitly issuing make enums.

Good point, this should keep the power of glib-mkenums available to the unix-like user

I can draft a proposal for this, if you want. Oh yes please, thanks you very much.

jjceresa commented 5 years ago

I can draft a proposal for this, if you want.

Yes, thanks you very much. If you do so, please note that SED substitution seems useless as no such strings dl_s2, DL_S2 exits in input headers files ${instpatch_public_HEADERS} (it looks like a leftover).

${SED} 's/dl_s2/dls2/g\;s/DL_S2/DLS2/g'
derselbst commented 5 years ago

Ok, a developer can now execute make enums to generate new builtin_enums.[h|c].

If you do so, please note that SED substitution seems useless as no such strings dl_s2, DL_S2 exits in input headers files ${instpatch_public_HEADERS} (it looks like a leftover).

No. It is necessary, because the output generated by mkenums contains it (don't know why).