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

LOCALEDIR is used by EXIV2_ENABLE_BUILD_PO so make sure it is defined #33

Merged
merged 2 commits into from
Aug 22, 2017

Conversation

a17r
Copy link
Contributor

@a17r a17r commented Aug 20, 2017

Otherwise it is possible to install translations in / if EXIV2_ENABLE_NLS=OFF.

@a17r
Copy link
Contributor Author

a17r commented Aug 21, 2017

However, it might be worth checking if the variable incl win32 check can be dropped entirely for using gnuinstalldir path again. But I cannot test on Windows.

@a17r
Copy link
Contributor Author

a17r commented Aug 21, 2017

@piponazo I can squash both commits if you are in favor of it.

@clanmills
Copy link
Collaborator

@piponazo We discussed libintl and the library. tags.cpp and exiv2.cpp use libintl. tags.cpp is part of the library. exiv2.cpp is main() for the the exiv2 command-line-program. So, we need to link libintl with libexiv2.

581 rmills@rmillsmbp:~/gnu/github/exiv2 $ grep domain src/*.cpp
src/exiv2.cpp:    bindtextdomain(EXV_PACKAGE, EXV_LOCALEDIR);
src/exiv2.cpp:    textdomain(EXV_PACKAGE);
src/types.cpp:        bindtextdomain(EXV_PACKAGE, EXV_LOCALEDIR);
src/types.cpp:        bind_textdomain_codeset (EXV_PACKAGE, "UTF-8");
582 rmills@rmillsmbp:~/gnu/github/exiv2 $ 

Being a native English speaker, my brain isn't built to understand localisation.

@piponazo
Copy link
Collaborator

The changes look good, thanks @a17r !

the only thing I would try to check is to replace the add_definitions( -DEXV_LOCALEDIR="${CMAKE_INSTALL_LOCALEDIR}" ) by :

target_compile_definitions(exiv2lib PRIVATE EXV_LOCALEDIR="${CMAKE_INSTALL_LOCALEDIR}" )
target_compile_definitions(exiv2     PRIVATE EXV_LOCALEDIR="${CMAKE_INSTALL_LOCALEDIR}" )

In that way we do not add the definition to other things (the XMP static library, the sample applications, etc).

Could you try that @a17r ? If it is too much work, I can merge this as it is and take care of what I just described.

It is always properly defined by GNUInstallDirs. Otherwise it was
possible to install translations in / if EXIV2_ENABLE_NLS=OFF.

Thanks-to: Luis Díaz Más
@a17r
Copy link
Contributor Author

a17r commented Aug 21, 2017

Thanks, works fine for me.

@piponazo
Copy link
Collaborator

piponazo commented Aug 21, 2017

Thanks @a17r for taking care of the suggestion 👍

I will merge once travis finishes its jobs :)

I had to resolve some conflicts before merging into master

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.

3 participants