On Tue, Jun 10, 2025 at 12:26 PM Branko Čibej <br...@apache.org> wrote:
> On 10. 6. 25 17:32, Timofei Zhakov wrote: > > On Tue, Jun 10, 2025 at 5:24 PM Branko Čibej <br...@apache.org> wrote: > >> On 10. 6. 25 16:01, Timofei Zhakov 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 >>> 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_*_CHARSET family has nothing similar with APR_*_CHARSET since 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_*_CHARSET to our utf >> routines, we are all fine. >> >> >> Yes. "As long as". This stuff has been in our public header since before >> Subversion was an ASF project. APR is and always has been part of >> Subversion's public interface. >> >> It doesn't make us dependent on apr's constants. Please withdraw your veto. >> >> >> You're changing a public header in a backward-incompatible way because >> the CMake build has broken include paths. >> >> You don't see anything wrong with that? Looks to me like you got your >> priorities wrong. >> >> -- Brane >> >> > Okay, I see. You are talking about removing this include, not about the > whole change, right? I agree that this is wrong for backward compat to > remove this from the private header. However, the remaining part > (with SVN_APR_*_CHARSET) is correct. It's still an improvement IMO, which > we can use later to add new charset shortcuts (like I said for utf8). > > > No, it is not correct, because this is *not* a private header. It's public > interface. > > The proper fix would be to put something like this in CMakeLists.txt: > > include_directories($<TARGET_PROPERTY:apr::apr,INCLUDE_DIRECTORIES> > $<TARGET_PROPERTY:apr::aprutil,INCLUDE_DIRECTORIES>) > > > Right now, your changes for utf-8 argument parsing has spilled into our > public API in an incompatible way, just to avoid fixing cmake. > > > -- Brane > Hi, Timofei, your tremendous efforts are very much appreciated. Brane, your vigilance and care is very much appreciated too. Okay, so there's a speed bump here. Please, let's all be patient and try to gain some clarity, and let's try to stay positive. We've seen quite a bit of what *shouldn't* be done; let's try to focus on what *should* be done... Yes, obviously I agree that we should not change public APIs, nor mask any problem by patching the wrong thing, such as fixing a build problem. Btw I think build system issues are not the motivation for this change... It looks to me like this patch is meant to be one step in a sequence, but we haven't seen the rest of the sequence, because we've paused here. So we're not understanding the full picture in context. So, if we could understand what the rest of the sequence will be? What problem are we trying to solve? What is the intended solution? >From my reading of the diff: It looks like the intention is to "subclass" and extend APR's charset stuff by doing two things: (1) defining SVN_APR_LOCALE_CHARSET and SVN_APR_DEFAULT_CHARSET "on top of" APR_LOCALE_CHARSET and APR_DEFAULT_CHARSET, to set the stage for adding further definitions (which we haven't seen yet), and (2) adding the function get_apr_xlate_charset(), which currently is a no-op, but appears as though it intends to be extended later to examine CHARSET and either pass it through unchanged or translate it to a different code. Am I correct above the above? If not, where am I mistaken? In APR, APR_LOCALE_CHARSET and APR_DEFAULT_CHARSET are #define'd to 0 and 1, respectively, typecast to (const char *). The reason for this typecasting is so that you can either pass one of the above names, or pass a string, like "ISO-8559-1". Several things are not clear to me: (1) How does APR intend for applications to extend beyond these two codes? Perhaps it's one of these possibilities: (a) APR intends for applications to use either APR_LOCALE_CHARSET and APR_DEFAULT_CHARSET or use strings. Nothing else. Or, (b) APR intends for applications to add their own defines, and perhaps use the above defines and/or strings also. Or, (c) APR hasn't considered this question. Or, (d) APR hasn't considered this question because it should never be necessary to do so. If so, what *are* we supposed to do? Brane's insights here will be particularly appreciated. (2) Perhaps more importantly, does APR lack something that SVN needs in this area? If so, what? Now, a suggestion. A picture is worth a thousand words, and in software development, code is pictures. So perhaps things would be more clear to everyone by doing something like: (1) Revert the change on trunk. (2) Fix the CMake build if indeed it's broken right now; I saw Brane's response but I haven't checked. Fixing it will remove anything CMake-related from the equation, so we can focus on the charset issue without doubts. (3) Create a branch and make this change + the further changes that are supposed to build upon it on the branch... Now, with a branch and with all the changes together, we will all be a lot smarter because we will see everything in context. Right now, we're seeing one change in a vacuum and making judgments without the full information. Even if the branch turns out to approach the problem from the wrong direction, at least we'll be able to see the full picture and get some better feedback about how to do it right. Again, I agree about not changing the public APIs. That's what we should *not* do. I'm trying to get at: what *should* we do? Thoughts? Ideas? Possibilities? Cheers, Nathan
