<table><tr><td style="">sitter added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D5167" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>The code currently isn't working for po/ in source dir and you are losing a public function. See inline comments.</p>
<p>(Why there are no autotests for a complicated, involved, and important function like ki18n_install is entirely beyond my appreciation. How in the world is anyone meant to be able to confidently change or even review a change to this if there are a million cases to consider that are not even written down as a spec and cannot be readily tested as you first have to look for bloody real world examples where this stuff appears. This is making me so 😠)</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5167#inline-21922" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">KF5I18NMacros.cmake:62</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d">#usage: KI18N_INSTALL_TS_FILES("ja" ${scripts_dir})</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">function</span><span class="p">(</span><span style="color: #766510">KI18N_INSTALL_TS_FILES</span> <span style="color: #766510">lang</span> <span style="color: #766510">scripts_dir</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #304a96">file</span><span class="p">(</span><span style="color: #766510">GLOB_RECURSE</span> <span style="color: #766510">ts_files</span> <span style="color: #766510">RELATIVE</span> <span style="color: #aa2211">${</span><span style="color: #001294">CMAKE_CURRENT_SOURCE_DIR</span><span style="color: #aa2211">}</span> <span style="color: #aa2211">${</span><span style="color: #001294">scripts_dir</span><span style="color: #aa2211">}</span><span style="color: #766510">/*</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">It seems you are losing the public <tt style="background: #ebebeb; font-size: 13px;">function(KI18N_INSTALL_TS_FILES</tt> seeing as that was public I would think a compat function calling the centralized ki18n_install function is needed + deprecation warning and note about deprecatedness in documentation please (:</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5167#inline-21919" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">KF5I18NMacros.cmake:101</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #304a96">string</span><span class="p">(</span><span style="color: #766510">MD5</span> <span style="color: #766510">pathmd5</span> <span style="color: #aa2211">${</span><span style="color: #001294">podir</span><span style="color: #aa2211">}</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I do not think that this is good enough.</p>
<p style="padding: 0; margin: 8px;">Say I have a tree like this:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">.
├── CMakeLists.txt
├── po
└── subsrc
├── CMakeLists.txt
└── po</pre></div>
<p style="padding: 0; margin: 8px;">I would be calling ki18n_install twice with the same podir argument <tt style="background: #ebebeb; font-size: 13px;">ki18n_install(po)</tt>, so your string md5 ends up the same and cmake will fail due to name overlap in the targets. They are in different CURRENT_SOURCE_DIR contexts though.<br />
What you need to do is something like</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">get_filename_component(abs podir ABSOLUTE)`
file(RELATIVE_PATH rel ${CMAKE_SOURCE_DIR} ${abs})</pre></div>
<p style="padding: 0; margin: 8px;">to resolve the path of podir relative to the source and use that to drive the md5 calculation as that path should then be unique (unless of course one calls ki18n_install in the same CMakeLists.txt more than once, which arguably is using it incorrectly ^^)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5167#inline-21921" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">KF5I18NMacros.cmake:121</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #304a96"><span class="bright">if</span></span><span class="p">(</span><span style="color: #766510">NOT</span> <span style="color: #766510">TARGET</span> <span style="color: #766510">pofiles</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"> </span><span style="color: #304a96"><span class="bright">if</span></span><span class="bright"> </span><span class="p">(</span><span style="color: #766510">NOT</span> <span style="color: #766510">TARGET</span> <span style="color: #766510">pofiles</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #304a96">add_custom_target</span><span class="p">(</span><span style="color: #766510">pofiles</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">style: no space between if and (</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5167#inline-21918" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">build-pofiles.cmake:27</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">file</span><span class="p">(</span><span style="color: #766510">GLOB_RECURSE</span> <span style="color: #766510">pofiles</span> <span style="color: #766510">RELATIVE</span> <span style="color: #766510">"${PO_DIR}"</span> <span style="color: #766510">"${PO_DIR}/**.po"</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This does not work. As the pofiles-a1b2c3 target is forking another cmake the current source dir here is the binary dir not the original source dir, so while this works for the new fetch-translations target which puts the po in $bindir/po, it breaks for actually released software which will need the translations to be found in $srcdir/po.</p>
<p style="padding: 0; margin: 8px;">I think the only reasonable way to solve this is to have KI18N_INSTALL create a target for src and one for bin and then pass in the absolute paths of each.</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">set(podirs "${podir}")
if(NOT IS_ABSOLUTE ${podir})
set(bin_podir "${CMAKE_CURRENT_SOURCE_DIR}/${podir}")
set(src_podir "${CMAKE_CURRENT_BINARY_DIR}/${podir}")
set(podirs "${bin_podir};${src_podir}")
list(REMOVE_DUPLICATES podirs) # src == bin
endif()
foreach(podir IN LISTS podirs)
string(MD5 pathmd5 ${podir})
add_custom_target(pofiles-${pathmd5} ALL
.......</pre></div>
<p style="padding: 0; margin: 8px;">If only one of them exists the other target will simply create nothing as the glob comes back empty. If both exist both create potentially the same targets, which may be undesirable, so perhaps this code should live in the build*.cmake helper files and instead of creating two targets you simply pass in the <tt style="background: #ebebeb; font-size: 13px;">podirs</tt> list. The helpers then glob both in an exclusive fashion (i.e. foreach until one of the list entries comes back with a non-empty GLOB).</p>
<p style="padding: 0; margin: 8px;">i.e. in <tt style="background: #ebebeb; font-size: 13px;">function(KI18N_INSTALL</tt>:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">set(podirs "${podir}")
if(NOT IS_ABSOLUTE ${podir})
set(bin_podir "${CMAKE_CURRENT_SOURCE_DIR}/${podir}")
set(src_podir "${CMAKE_CURRENT_BINARY_DIR}/${podir}")
set(podirs "${bin_podir};${src_podir}")
list(REMOVE_DUPLICATES podirs) # src == bin
endif()</pre></div>
<p style="padding: 0; margin: 8px;">and passing <tt style="background: #ebebeb; font-size: 13px;"> -DPO_DIRS=${podirs}</tt></p>
<p style="padding: 0; margin: 8px;">and in the helpers:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">foreach(podir IN LISTS PO_DIRS)
file(GLOB_RECURSE pofiles RELATIVE "${podir}" "${podir}/**.po")
list(LENGTH pofiles pofiles_length)
if(${pofiles_length} GREATER 0)
set(PO_DIR ${podir})
break()
endif()
endforeach()</pre></div>
<p style="padding: 0; margin: 8px;">For testing this you can use a tag of plasma-framework for example (that also has a scripts dir FWIW).</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R249 KI18n</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D5167" rel="noreferrer">https://phabricator.kde.org/D5167</a></div></div><br /><div><strong>To: </strong>apol, Frameworks, ltoscano, ilic, sitter<br /><strong>Cc: </strong>aacid<br /></div>