On 10. 6. 25 17:26, Nathan Hartman wrote:
On Tue, Jun 10, 2025 at 10:07 AM Timofei Zhakov <t...@chemodax.net> wrote:On Mon, Jun 9, 2025 at 7:26 PM Branko Čibej <br...@apache.org> wrote: On 9. 6. 25 18:31, rin...@apache.org wrote:Author: rinrab Date: Mon Jun 9 16:31:15 2025 New Revision: 1926293 URL:http://svn.apache.org/viewvc?rev=1926293&view=rev <http://svn.apache.org/viewvc?rev=1926293&view=rev> Log: Do not include apr_xlate.h into svn_utf.h, but manually wrap the SVN_APR_*_CHARSET constants into APR_*_CHARSET.-1. You can't presume that this will never change in APR. Fix the cmake build to handle include paths correctly. This is not that fix. Please revert. -- Brane I want to make sure that you entirely understand this change. SVN_APR_*_CHARSETfamily has nothing similar with APR_*_CHARSETsince they are being converted into APR constants through the get_apr_xlate_charset()function. This also means we can add our own shortcuts to SVN_APR_*_CHARSET, which I'm planning to do for UTF-8. As long as consumers of our library are not passing APR_*_CHARSETto our utf routines, we are all fine. It doesn't make us dependent on apr's constants. Please withdraw your veto. Is there indeed a build system problem related to including apr_xlate.h?
Yes. The cmake build adds the apr-util include paths only when compiling those targets that link with apr-util. The autotools build doesn't do that, even though the two use the same dependency generator.
I'll pre-empt the "this is how cmake works" argument by saying that the cmake build should be functionally equivalent to the autotools build. Either that, or declare that the cmake build works only on Windows with vcpkg dependencies and nowhere else. I'm fine with that.
As I'm reading the diff, it seems the purpose is to decouple from APR, to lay groundwork for later changes that will also add new SVN_*_CHARSET constants that aren't provided by APR?
There are only two constants provided by APR and Subversion has used those two constants since forever. And exposed them in a public header (along with other APR stuff).
If so, then perhaps removing "APR" from the names of the constants could help avoid confusion?
It's not about confusion at all. The build is broken. You don't "fix" a broken build by "fixing" our public API. You can change the implementation to ignore those constants, or add new constants and deprecate the old ones, but you sure can't just change their meaning.
-- Brane
