RE: svn commit: r40417 - in trunk: . subversion/include subversion/include/private subversion/libsvn_client subversion/libsvn_subr subversion/libsvn_wc

[Bert Huijben-6]

> -----Original Message-----
> From: Arfrever Frehtes Taifersar Arahesis
> [mailto:Arfrever.FTA@...]
> Sent: zaterdag 7 november 2009 12:14
> To: svn@...
> Subject: svn commit: r40417 - in trunk: . subversion/include
> subversion/include/private subversion/libsvn_client
> subversion/libsvn_subr subversion/libsvn_wc
>
> Author: arfrever
> Date: Fri Nov  6 15:14:00 2009
> New Revision: 40417
>
> Log:
> Fix inclusion of svn_debug.h. Previously this private header was
> included from
> public header (svn_types.h) when SVN_DEBUG was defined. It was causing
> build
> failures when including installed svn_types.h with SVN_DEBUG defined.
> Now
> svn_debug.h is automatically included when SVN_DEBUG is defined during
> building
> of Subversion, but isn't included from installed svn_types.h when
> SVN_DEBUG is
> defined.

What do you try to fix?

SVN_DEBUG is an internal switch we use for debugging subversion itself. It should not be set by user code. And our own compilation should never use an installed svn_types.h, or we can never change this file again.

This breaks my debug/test builds where I use SVN_DBG() in source files (e.g. all over libsvn_wc). The follow-ups also break exporting the debug helpers on windows... it makes it impossible to use SVN_DBG() in a shared build?

It also breaks a static debug compilation in dso.c (in libsvn_subr)

#ifdef SVN_DEBUG
          char buf[1024];
          SVN_DBG(("%s\n", apr_dso_error(*dso, buf, 1024)));
#endif


So Again: What problem did you want to fix?

Did you try this patch in debug and release builds?
And did you try using SVN_DBG() in a file like update_editor.c? This is why this file was added.

We intended never to commit usages of SVN_DBG(), but it was not added as something we will never use.

Are you going to fix this?



        Bert

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415456

[Arfrever Frehtes Taifersar Arahesis-2]

2009-11-07 20:27:54 Bert Huijben napisał(a):

> > -----Original Message-----
> > From: Arfrever Frehtes Taifersar Arahesis
> > [mailto:Arfrever.FTA@...]
> > Sent: zaterdag 7 november 2009 12:14
> > To: svn@...
> > Subject: svn commit: r40417 - in trunk: . subversion/include
> > subversion/include/private subversion/libsvn_client
> > subversion/libsvn_subr subversion/libsvn_wc
> >
> > Author: arfrever
> > Date: Fri Nov  6 15:14:00 2009
> > New Revision: 40417
> >
> > Log:
> > Fix inclusion of svn_debug.h. Previously this private header was
> > included from
> > public header (svn_types.h) when SVN_DEBUG was defined. It was causing
> > build
> > failures when including installed svn_types.h with SVN_DEBUG defined.
> > Now
> > svn_debug.h is automatically included when SVN_DEBUG is defined during
> > building
> > of Subversion, but isn't included from installed svn_types.h when
> > SVN_DEBUG is
> > defined.
>
>
> What do you try to fix?

Public headers shouldn't include private headers.

> SVN_DEBUG is an internal switch we use for debugging subversion itself. It should not be set by user code. And our own compilation should never use an installed svn_types.h, or we can never change this file again.
>
> This breaks my debug/test builds where I use SVN_DBG() in source files (e.g. all over libsvn_wc).

What exactly errors do you get? Does '#include "private/svn_debug.h"' avoid them?

> The follow-ups also break exporting the debug helpers on windows...

Can the debug helpers be used if svn_debug.h is directly included?

> it makes it impossible to use SVN_DBG() in a shared build?

This commit should identically affect shared and static build.

> It also breaks a static debug compilation in dso.c (in libsvn_subr)

What error exactly?

> #ifdef SVN_DEBUG
>           char buf[1024];
>           SVN_DBG(("%s\n", apr_dso_error(*dso, buf, 1024)));
> #endif
>
>
> So Again: What problem did you want to fix?
>
> Did you try this patch in debug and release builds?

Yes, but not on Windows.

> And did you try using SVN_DBG() in a file like update_editor.c?

It works without problems for me (in debug mode).

--
Arfrever Frehtes Taifersar Arahesis

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415497

[Branko Cibej]

Arfrever Frehtes Taifersar Arahesis wrote:
> 2009-11-07 20:27:54 Bert Huijben napisał(a):
>  
>> What do you try to fix?
>>    
>
> Public headers shouldn't include private headers.
>
>  
I find myself agreeing in principle with Arfrever; however, the "fix" is
totally bogus because it breaks debug builds for anyone who's not using
GCC as their compiler. At the very least, you should test if your
preprocessor even supports the -include option, not just blindly assume
that any system that uses configure also uses GCC.

Next: What on earth is wrong with unconditionally including
private/svn_debug.h from an implementation file? You had to remove those
includes only because you added that #error directove in svn_debug;
which is totally unnecessary. It should be perfectly safe and acceptable
for a Subversion C file to include svn_debug.h in a non-debug build.

Next: Same goes for your handling of SQLITE3_DEBUG.

Verdict: -1, please revert this change. The principle you quote above
can be served simply by requiring implementation files that happen to
think they need SVN_DBG to include private/svn_debug.h explicitly. In
any case, your change breaks things for any number of people who don't
compile with GCC.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415522

[Stefan Sperling-7]

On Sun, Nov 08, 2009 at 07:30:36AM +0100, Branko Cibej wrote:
> Verdict: -1, please revert this change.

+1 on revert, and simply removing #include <svn_debug.h> from
svn_types.h and requiring people who use SVN_DBG to include svn_debug.h
manually.

The change also broke the perl bindings build:
cc1: subversion/include/private/svn_debug.h: No such file or directory

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415536

[Arfrever Frehtes Taifersar Arahesis-2]

2009-11-08 11:17:39 Stefan Sperling napisał(a):
> The change also broke the perl bindings build:
> cc1: subversion/include/private/svn_debug.h: No such file or directory

This was fixed in r40419.

--
Arfrever Frehtes Taifersar Arahesis

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415564

[Branko Cibej]

Arfrever Frehtes Taifersar Arahesis wrote:
> 2009-11-08 11:17:39 Stefan Sperling napisał(a):
>  
>> The change also broke the perl bindings build:
>> cc1: subversion/include/private/svn_debug.h: No such file or directory
>>    
>
> This was fixed in r40419

You're missing the point. This change has no effect on the technical
reasons for my veto.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415565

[Philip Martin]

Arfrever Frehtes Taifersar Arahesis <Arfrever.FTA@...> writes:

> What exactly errors do you get? Does '#include "private/svn_debug.h"' avoid them?

$ ../src/configure --enable-debug
...
checking for readlink... yes
checking zlib.h usability... no
checking zlib.h presence... no
checking for zlib.h... no
configure: error: subversion requires zlib

From config.log:

configure:11619: checking zlib.h usability
configure:11636: gcc -c -g -pthread -Werror=implicit-function-declaration  -DSVN_DEBUG -DAP_DEBUG   -DLINUX=2 -D_REENTRANT -D_GNU_SOURCE -include subversion/include/private/svn_debug.h conftest.c >&5
cc1: error: subversion/include/private/svn_debug.h: No such file or directory

--
Philip

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415605

[Philip Martin]

Stefan Sperling <stsp@...> writes:

> +1 on revert, and simply removing #include <svn_debug.h> from
> svn_types.h and requiring people who use SVN_DBG to include svn_debug.h
> manually.

That sounds sensible.

--
Philip

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415606

[Arfrever Frehtes Taifersar Arahesis-2]

2009-11-08 14:51:50 Branko Čibej napisał(a):

> Arfrever Frehtes Taifersar Arahesis wrote:
> > 2009-11-08 11:17:39 Stefan Sperling napisał(a):
> >  
> >> The change also broke the perl bindings build:
> >> cc1: subversion/include/private/svn_debug.h: No such file or directory
> >>    
> >
> > This was fixed in r40419
>
> You're missing the point. This change has no effect on the technical
> reasons for my veto.

I'm not missing any point, because I haven't said that your arguments are
incorrect. I only didn't have time to revert these changes.
I reverted wrong changes in r40423.

--
Arfrever Frehtes Taifersar Arahesis

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415618

[Daniel Shahaf-2]

Arfrever Frehtes Taifersar Arahesis wrote on Sun, 8 Nov 2009 at 23:03 +0100:
> I'm not missing any point, because I haven't said that your arguments are
> incorrect. I only didn't have time to revert these changes.

It would be extremely helpful if you communicated that you acknowledge the
veto (i.e., that you know it was uttered).  By ignoring it completely you
leave a very bad impression that can be very easily misinterpreted.

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415623

[Bert Huijben-6]

> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@...]
> Sent: zondag 8 november 2009 23:26
> To: Arfrever Frehtes Taifersar Arahesis
> Cc: Branko Čibej; Stefan Sperling; Subversion Development; Bert Huijben
> Subject: Re: svn commit: r40417 - in trunk: . subversion/include
> subversion/include/private subversion/libsvn_client
> subversion/libsvn_subr subversion/libsvn_wc
>
> Arfrever Frehtes Taifersar Arahesis wrote on Sun, 8 Nov 2009 at 23:03
> +0100:
> > I'm not missing any point, because I haven't said that your arguments
> are
> > incorrect. I only didn't have time to revert these changes.
>
> It would be extremely helpful if you communicated that you acknowledge
> the
> veto (i.e., that you know it was uttered).  By ignoring it completely
> you
> leave a very bad impression that can be very easily misinterpreted.

I reverted some further changes on subversion/libsvn_subr/debug.c which
broke the shared library support on Windows, per the hacking.html rule:
"To prevent loss of productivity, any committer (full or partial) can
immediately revert any build system change that breaks their ability to
effectively do development on their platform of choice, as a matter of
ordinary routing, without fear of accusations of an over-reaction. The log
message of the commit reverting the change should contain an explanatory
note saying why the change is being reverted, containing sufficient detail
to be suitable to start off a discussion of the problem on dev@, should
someone chose to reply to the commit mail."

The extractor.py changes were reverted, which added the functions back to
the public dll api.. but the functions itself were not defined in release
mode.

        Bert

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415738

[Branko Cibej]

Bert Huijben wrote:
> The extractor.py changes were reverted, which added the functions back to
> the public dll api.. but the functions itself were not defined in release
> mode.
>  

That is a correct change. We should *always* have all functions defined
and available in libraries, regardless of whether they're "disabled" by
some macro or not. This allows, e.g., a debug build of some file to link
with release libraries and not freak out in the process.

(Doing this wrong first bit me with some early version of MS Visual
Studio (6 maybe?) where you couldn't make a release build without
defining NDEBUG because the function that implemented the assert() macro
was not linked into their release library. I learned a few new swear
words then.)


-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415744

[Greg Stein-4]

On Sun, Nov 8, 2009 at 05:17, Stefan Sperling <stsp@...> wrote:
> On Sun, Nov 08, 2009 at 07:30:36AM +0100, Branko Cibej wrote:
>> Verdict: -1, please revert this change.
>
> +1 on revert, and simply removing #include <svn_debug.h> from
> svn_types.h and requiring people who use SVN_DBG to include svn_debug.h
> manually.

I put the include into svn_types.h to make our lives easier. Without
that, then we'll end up with svn_debug.h scattered around (as noted by
Arfrever's removal of them).

There should be NO PROBLEM including a private header from a public
one, if that inclusion only happens *for us*.


And can I just say: -1 also on all these changes. This is a pretty
crappy "solution" to whatever the original problem might have been.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415784

[Greg Stein-4]

On Mon, Nov 9, 2009 at 05:50, Branko Cibej <brane@...> wrote:
> Bert Huijben wrote:
>> The extractor.py changes were reverted, which added the functions back to
>> the public dll api.. but the functions itself were not defined in release
>> mode.
>
> That is a correct change. We should *always* have all functions defined
> and available in libraries, regardless of whether they're "disabled" by
> some macro or not. This allows, e.g., a debug build of some file to link
> with release libraries and not freak out in the process.

Yeah. Bert and I talked about this. Keep the functions in the public
API, but keep them hidden like our other private APIs. This could
happen if third-party developers wanted to be sneaky and use our debug
functions for their code, but against release libraries.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415786

[Branko Cibej]

Greg Stein wrote:

> On Sun, Nov 8, 2009 at 05:17, Stefan Sperling <stsp@...> wrote:
>  
>> On Sun, Nov 08, 2009 at 07:30:36AM +0100, Branko Cibej wrote:
>>    
>>> Verdict: -1, please revert this change.
>>>      
>> +1 on revert, and simply removing #include <svn_debug.h> from
>> svn_types.h and requiring people who use SVN_DBG to include svn_debug.h
>> manually.
>>    
>
> I put the include into svn_types.h to make our lives easier. Without
> that, then we'll end up with svn_debug.h scattered around (as noted by
> Arfrever's removal of them).
>
> There should be NO PROBLEM including a private header from a public
> one, if that inclusion only happens *for us*.
>  

Well ... Yeah, I guess, sure, as long as no-one who builds tools on top
of an installed Subversion #defines SVN_DEBUG in their build. Now I know
of no good reason to do such a thing; but it could happen.

So in the end, its a choice between telling said user in an ugly way
that she should leave off defining SVN_DEBUG (file not found:
private/svn_debug.h), or just silently ignoring that. Or generating
svn_types.h.

Frankly at this point I can't be bothered to think things through enough
to care; as long as the "fix" got reverted.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415789

[Greg Stein-4]

On Mon, Nov 9, 2009 at 09:01, Branko Čibej <brane@...> wrote:

> Greg Stein wrote:
>> On Sun, Nov 8, 2009 at 05:17, Stefan Sperling <stsp@...> wrote:
>>
>>> On Sun, Nov 08, 2009 at 07:30:36AM +0100, Branko Cibej wrote:
>>>
>>>> Verdict: -1, please revert this change.
>>>>
>>> +1 on revert, and simply removing #include <svn_debug.h> from
>>> svn_types.h and requiring people who use SVN_DBG to include svn_debug.h
>>> manually.
>>>
>>
>> I put the include into svn_types.h to make our lives easier. Without
>> that, then we'll end up with svn_debug.h scattered around (as noted by
>> Arfrever's removal of them).
>>
>> There should be NO PROBLEM including a private header from a public
>> one, if that inclusion only happens *for us*.
>>
>
> Well ... Yeah, I guess, sure, as long as no-one who builds tools on top
> of an installed Subversion #defines SVN_DEBUG in their build. Now I know
> of no good reason to do such a thing; but it could happen.
>
> So in the end, its a choice between telling said user in an ugly way
> that she should leave off defining SVN_DEBUG (file not found:
> private/svn_debug.h), or just silently ignoring that. Or generating
> svn_types.h.
>
> Frankly at this point I can't be bothered to think things through enough
> to care; as long as the "fix" got reverted.

hehe...

Well, nobody should be defining symbols in our namespace. I think our
API story has always been pretty clear on that. So I'm quite fine with
it failing for people when they do that.

And meanwhile: we get easier development.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415793

[Arfrever Frehtes Taifersar Arahesis-2]

2009-11-09 14:51:28 Greg Stein napisał(a):

> On Sun, Nov 8, 2009 at 05:17, Stefan Sperling <stsp@...> wrote:
> > On Sun, Nov 08, 2009 at 07:30:36AM +0100, Branko Cibej wrote:
> >> Verdict: -1, please revert this change.
> >
> > +1 on revert, and simply removing #include <svn_debug.h> from
> > svn_types.h and requiring people who use SVN_DBG to include svn_debug.h
> > manually.
>
> I put the include into svn_types.h to make our lives easier. Without
> that, then we'll end up with svn_debug.h scattered around (as noted by
> Arfrever's removal of them).

Why don't you use svn_private_config.h? It's included in even more files
than svn_types.h:

$ grep -Er '#include[[:space:]]+"svn_private_config\.h"' subversion | grep -v /\.svn/ | wc -l
273
$ grep -Er '#include[[:space:]]+"svn_types\.h"' subversion | grep -v /\.svn/ | wc -l
168

--
Arfrever Frehtes Taifersar Arahesis

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417412

[Greg Stein-4]

On Fri, Nov 13, 2009 at 00:06, Arfrever Frehtes Taifersar Arahesis
<arfrever.fta@...> wrote:

> 2009-11-09 14:51:28 Greg Stein napisał(a):
>> On Sun, Nov 8, 2009 at 05:17, Stefan Sperling <stsp@...> wrote:
>> > On Sun, Nov 08, 2009 at 07:30:36AM +0100, Branko Cibej wrote:
>> >> Verdict: -1, please revert this change.
>> >
>> > +1 on revert, and simply removing #include <svn_debug.h> from
>> > svn_types.h and requiring people who use SVN_DBG to include svn_debug.h
>> > manually.
>>
>> I put the include into svn_types.h to make our lives easier. Without
>> that, then we'll end up with svn_debug.h scattered around (as noted by
>> Arfrever's removal of them).
>
> Why don't you use svn_private_config.h? It's included in even more files
> than svn_types.h:
>
> $ grep -Er '#include[[:space:]]+"svn_private_config\.h"' subversion | grep -v /\.svn/ | wc -l
> 273
> $ grep -Er '#include[[:space:]]+"svn_types\.h"' subversion | grep -v /\.svn/ | wc -l
> 168

Hadn't thought about that solution. That should work fine.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417414

[Bert Huijben-6]

On Fri, Nov 13, 2009 at 6:06 AM, Arfrever Frehtes Taifersar Arahesis
<arfrever.fta@...> wrote:

> 2009-11-09 14:51:28 Greg Stein napisał(a):
>> On Sun, Nov 8, 2009 at 05:17, Stefan Sperling <stsp@...> wrote:
>> > On Sun, Nov 08, 2009 at 07:30:36AM +0100, Branko Cibej wrote:
>> >> Verdict: -1, please revert this change.
>> >
>> > +1 on revert, and simply removing #include <svn_debug.h> from
>> > svn_types.h and requiring people who use SVN_DBG to include svn_debug.h
>> > manually.
>>
>> I put the include into svn_types.h to make our lives easier. Without
>> that, then we'll end up with svn_debug.h scattered around (as noted by
>> Arfrever's removal of them).
>
> Why don't you use svn_private_config.h? It's included in even more files
> than svn_types.h:
>
> $ grep -Er '#include[[:space:]]+"svn_private_config\.h"' subversion | grep -v /\.svn/ | wc -l
> 273
> $ grep -Er '#include[[:space:]]+"svn_types\.h"' subversion | grep -v /\.svn/ | wc -l
> 168

I like the idea of using svn_private_config.h, but these numbers by
itself don't tell the full story. Almost all of our headers include
svn_types.h, so you don't have to include it directly. And you are
only counting direct includes.

   Bert
>
> --
> Arfrever Frehtes Taifersar Arahesis
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2418141