You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Robert Middleton <rm...@apache.org> on 2022/01/01 20:11:30 UTC
[log4cxx] Short filename options
PR #75 adds a new option for displaying the short filename of the log
location, which is probably a good idea to have, as in my experience,
the whole path of the file is not that useful, especially when the
binary is from a build server where the path is something like
/var/lib/jenkins/project-name/src/main/directory/foo.cpp.
The current PR does this with some string manipulation at runtime, and
is different from the const char* that is currently used, so it
doesn't fit that well with the rest of the class. Since C++11 we can
now do constexpr functions, so we should be able to do this at compile
time(assuming you have optimizations turned on of course).
We can do this one of several ways:
1. Take the PR as is(use strings and calculate at runtime)
2. Create a constexpr function that we control to calculate the
filename at compile time as either an offset into the filename or a
separate const char*. The advantage to this is that it doesn't
require any other libraries for pre-C++17 compilers.
3. Use std::string_view(for C++17) or boost::string_view(pre-c++17).
The following would work for this solution:
std::string_view str{"/foo/bar/what.cpp"};
const int location = str.find_last_of( '/' ) + 1;
printf( "fullPath: %s\n", str.data() );
printf( "fileName: %s\n", &str[location] );
Does anybody have a preference or a better way to do it? I'm inclined
to go with (3), since that does provide a good fallback for compilers
that don't support C++17, and only requires boost for older compilers.
-Robert Middleton
Re: [log4cxx] Short filename options
Posted by Robert Middleton <rm...@apache.org>.
Thinking about this a bit more still, I think we can do this in two
ways to make everything work properly. We can check for the
__cpp_lib_string_view macro and/or _MSVC_LANG(because windows always
needs to be different), and if the compiler supports std::string view
we use that at compile-time to pass in the const char* to the
LocationInfo class. If we can't do it at compile time, we will figure
it out at runtime inside of the LocationInfo. Either way this adds
one const char* to the LocationInfo class.
Regardless, the full path to the file is still embedded in the
binaries of users of the library unless you explicitly turn that off.
I'll add some documentation related to that.
-Robert Middleton
On Sun, Jan 2, 2022 at 8:07 AM Thorsten Schöning <ts...@am-soft.de> wrote:
>
> Guten Tag Robert Middleton,
> am Sonntag, 2. Januar 2022 um 02:38 schrieben Sie:
>
> > 2. Create a constexpr function that we control[...]
> > 3. Use std::string_view(for C++17) or
> > boost::string_view(pre-c++17).[...]
>
> Just to make sure I understand correctly: The difference between 2 and
> 3 is using a custom implementation vs. an already available one?
>
> As we rely on boost for whatever is not supported by the compiler
> anyway already, I prefer yur option 3 then. Looking at the PR, we had
> already three different implementations proposed in the end and I
> don't see any benefit to discuss those further when "string_view"
> handles all of this already.
>
> > [...]I can see certain
> > circumstances where it is useful to have the full path, for example
> > when you have two files named the same(please don't do this).
>
> I'm doing this sometimes and in theory it shouldn't be a problem if
> placed into different namespaces. Shouldn't it? Of course some broken
> IDEs/compilers like Embarcadero C++-Builder don't work properly with
> that in case of incremental builds, but that's not too much of a
> problem for me.
>
> Regarding absolute paths, one might want to keep in mind that not
> everyone ships results from automatic builds like Jenkins only and/or
> uses multiple different branches for different customers with slightly
> modified codebase at the same time. So keeping absolute paths if
> available before makes sense to me.
>
> Mit freundlichen Grüßen
>
> Thorsten Schöning
>
> --
> AM-SoFT IT-Service - Bitstore Hameln GmbH
> Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK
>
> E-Mail: Thorsten.Schoening@AM-SoFT.de
> Web: http://www.AM-SoFT.de/
>
> Tel: 05151- 9468- 0
> Tel: 05151- 9468-55
> Fax: 05151- 9468-88
> Mobil: 0178-8 9468-04
>
> AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln
> AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
>
>
>
>
Re: [log4cxx] Short filename options
Posted by Thorsten Schöning <ts...@am-soft.de>.
Guten Tag Robert Middleton,
am Sonntag, 2. Januar 2022 um 02:38 schrieben Sie:
> 2. Create a constexpr function that we control[...]
> 3. Use std::string_view(for C++17) or
> boost::string_view(pre-c++17).[...]
Just to make sure I understand correctly: The difference between 2 and
3 is using a custom implementation vs. an already available one?
As we rely on boost for whatever is not supported by the compiler
anyway already, I prefer yur option 3 then. Looking at the PR, we had
already three different implementations proposed in the end and I
don't see any benefit to discuss those further when "string_view"
handles all of this already.
> [...]I can see certain
> circumstances where it is useful to have the full path, for example
> when you have two files named the same(please don't do this).
I'm doing this sometimes and in theory it shouldn't be a problem if
placed into different namespaces. Shouldn't it? Of course some broken
IDEs/compilers like Embarcadero C++-Builder don't work properly with
that in case of incremental builds, but that's not too much of a
problem for me.
Regarding absolute paths, one might want to keep in mind that not
everyone ships results from automatic builds like Jenkins only and/or
uses multiple different branches for different customers with slightly
modified codebase at the same time. So keeping absolute paths if
available before makes sense to me.
Mit freundlichen Grüßen
Thorsten Schöning
--
AM-SoFT IT-Service - Bitstore Hameln GmbH
Mitglied der Bitstore Gruppe - Ihr Full-Service-Dienstleister für IT und TK
E-Mail: Thorsten.Schoening@AM-SoFT.de
Web: http://www.AM-SoFT.de/
Tel: 05151- 9468- 0
Tel: 05151- 9468-55
Fax: 05151- 9468-88
Mobil: 0178-8 9468-04
AM-SoFT IT-Service - Bitstore Hameln GmbH, Brandenburger Str. 7c, 31789 Hameln
AG Hannover HRB 221853 - Geschäftsführer: Janine Galonska
Re: [log4cxx] Short filename options
Posted by Stephen Webb <sw...@gmail.com>.
This is already provided by
```
#define LOG4CXX_LOCATION ::log4cxx::spi::LocationInfo()
#include <log4cxx/logger.h>
```
On Sun, Jan 2, 2022 at 12:38 PM Robert Middleton <rm...@apache.org>
wrote:
> The full path is already in the compiled code anyway, this would
> simply add the ability to get the filename from the full path(so
> another member to the LocationInfo class). I can see certain
> circumstances where it is useful to have the full path, for example
> when you have two files named the same(please don't do this).
>
> That does bring up a good point though, is that if you don't want
> location info compiled in, we should have a preprocessor macro that
> will compile out all of the location info data. We can already
> compile out log messages of certain levels, so hiding the location is
> just a natural extension of that.
>
> -Robert Middleton
>
> On Sat, Jan 1, 2022 at 7:50 PM Stephen Webb <sw...@gmail.com> wrote:
> >
> > I prefer option 2 if it is possible.
> >
> > I do not think it is useful to have the full path name (of the build
> > directory) in shipped binaries.
> >
> > On Sun, Jan 2, 2022 at 7:11 AM Robert Middleton <rm...@apache.org>
> > wrote:
> >
> > > PR #75 adds a new option for displaying the short filename of the log
> > > location, which is probably a good idea to have, as in my experience,
> > > the whole path of the file is not that useful, especially when the
> > > binary is from a build server where the path is something like
> > > /var/lib/jenkins/project-name/src/main/directory/foo.cpp.
> > >
> > > The current PR does this with some string manipulation at runtime, and
> > > is different from the const char* that is currently used, so it
> > > doesn't fit that well with the rest of the class. Since C++11 we can
> > > now do constexpr functions, so we should be able to do this at compile
> > > time(assuming you have optimizations turned on of course).
> > >
> > > We can do this one of several ways:
> > > 1. Take the PR as is(use strings and calculate at runtime)
> > > 2. Create a constexpr function that we control to calculate the
> > > filename at compile time as either an offset into the filename or a
> > > separate const char*. The advantage to this is that it doesn't
> > > require any other libraries for pre-C++17 compilers.
> > > 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17).
> > > The following would work for this solution:
> > > std::string_view str{"/foo/bar/what.cpp"};
> > > const int location = str.find_last_of( '/' ) + 1;
> > > printf( "fullPath: %s\n", str.data() );
> > > printf( "fileName: %s\n", &str[location] );
> > >
> > > Does anybody have a preference or a better way to do it? I'm inclined
> > > to go with (3), since that does provide a good fallback for compilers
> > > that don't support C++17, and only requires boost for older compilers.
> > >
> > > -Robert Middleton
> > >
>
Re: [log4cxx] Short filename options
Posted by Tobias Frost <to...@debian.org>.
On Sat, Jan 01, 2022 at 08:38:43PM -0500, Robert Middleton wrote:
> The full path is already in the compiled code anyway, this would
> simply add the ability to get the filename from the full path(so
> another member to the LocationInfo class). I can see certain
> circumstances where it is useful to have the full path, for example
> when you have two files named the same(please don't do this).
>
> That does bring up a good point though, is that if you don't want
> location info compiled in, we should have a preprocessor macro that
> will compile out all of the location info data. We can already
> compile out log messages of certain levels, so hiding the location is
> just a natural extension of that.
Current log4cxx is reproducible [1] as in reproducible-builds.org, and one
part of checking reproducibilty is to build in different location, it
looks like that current log4cxx does NOT embed the complete path.
So a wish from the Debian mainainer:
It would be nice to retain reproduciblity when implementing this feature...
A happy new year!
--
tobi
[1] https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/log4cxx.html
(0.12.1 is currently not reproducible due to something doxygen is doing, but
the library package is.)
> -Robert Middleton
>
> On Sat, Jan 1, 2022 at 7:50 PM Stephen Webb <sw...@gmail.com> wrote:
> >
> > I prefer option 2 if it is possible.
> >
> > I do not think it is useful to have the full path name (of the build
> > directory) in shipped binaries.
> >
> > On Sun, Jan 2, 2022 at 7:11 AM Robert Middleton <rm...@apache.org>
> > wrote:
> >
> > > PR #75 adds a new option for displaying the short filename of the log
> > > location, which is probably a good idea to have, as in my experience,
> > > the whole path of the file is not that useful, especially when the
> > > binary is from a build server where the path is something like
> > > /var/lib/jenkins/project-name/src/main/directory/foo.cpp.
> > >
> > > The current PR does this with some string manipulation at runtime, and
> > > is different from the const char* that is currently used, so it
> > > doesn't fit that well with the rest of the class. Since C++11 we can
> > > now do constexpr functions, so we should be able to do this at compile
> > > time(assuming you have optimizations turned on of course).
> > >
> > > We can do this one of several ways:
> > > 1. Take the PR as is(use strings and calculate at runtime)
> > > 2. Create a constexpr function that we control to calculate the
> > > filename at compile time as either an offset into the filename or a
> > > separate const char*. The advantage to this is that it doesn't
> > > require any other libraries for pre-C++17 compilers.
> > > 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17).
> > > The following would work for this solution:
> > > std::string_view str{"/foo/bar/what.cpp"};
> > > const int location = str.find_last_of( '/' ) + 1;
> > > printf( "fullPath: %s\n", str.data() );
> > > printf( "fileName: %s\n", &str[location] );
> > >
> > > Does anybody have a preference or a better way to do it? I'm inclined
> > > to go with (3), since that does provide a good fallback for compilers
> > > that don't support C++17, and only requires boost for older compilers.
> > >
> > > -Robert Middleton
> > >
Re: [log4cxx] Short filename options
Posted by Robert Middleton <rm...@apache.org>.
The full path is already in the compiled code anyway, this would
simply add the ability to get the filename from the full path(so
another member to the LocationInfo class). I can see certain
circumstances where it is useful to have the full path, for example
when you have two files named the same(please don't do this).
That does bring up a good point though, is that if you don't want
location info compiled in, we should have a preprocessor macro that
will compile out all of the location info data. We can already
compile out log messages of certain levels, so hiding the location is
just a natural extension of that.
-Robert Middleton
On Sat, Jan 1, 2022 at 7:50 PM Stephen Webb <sw...@gmail.com> wrote:
>
> I prefer option 2 if it is possible.
>
> I do not think it is useful to have the full path name (of the build
> directory) in shipped binaries.
>
> On Sun, Jan 2, 2022 at 7:11 AM Robert Middleton <rm...@apache.org>
> wrote:
>
> > PR #75 adds a new option for displaying the short filename of the log
> > location, which is probably a good idea to have, as in my experience,
> > the whole path of the file is not that useful, especially when the
> > binary is from a build server where the path is something like
> > /var/lib/jenkins/project-name/src/main/directory/foo.cpp.
> >
> > The current PR does this with some string manipulation at runtime, and
> > is different from the const char* that is currently used, so it
> > doesn't fit that well with the rest of the class. Since C++11 we can
> > now do constexpr functions, so we should be able to do this at compile
> > time(assuming you have optimizations turned on of course).
> >
> > We can do this one of several ways:
> > 1. Take the PR as is(use strings and calculate at runtime)
> > 2. Create a constexpr function that we control to calculate the
> > filename at compile time as either an offset into the filename or a
> > separate const char*. The advantage to this is that it doesn't
> > require any other libraries for pre-C++17 compilers.
> > 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17).
> > The following would work for this solution:
> > std::string_view str{"/foo/bar/what.cpp"};
> > const int location = str.find_last_of( '/' ) + 1;
> > printf( "fullPath: %s\n", str.data() );
> > printf( "fileName: %s\n", &str[location] );
> >
> > Does anybody have a preference or a better way to do it? I'm inclined
> > to go with (3), since that does provide a good fallback for compilers
> > that don't support C++17, and only requires boost for older compilers.
> >
> > -Robert Middleton
> >
Re: [log4cxx] Short filename options
Posted by Stephen Webb <sw...@gmail.com>.
I prefer option 2 if it is possible.
I do not think it is useful to have the full path name (of the build
directory) in shipped binaries.
On Sun, Jan 2, 2022 at 7:11 AM Robert Middleton <rm...@apache.org>
wrote:
> PR #75 adds a new option for displaying the short filename of the log
> location, which is probably a good idea to have, as in my experience,
> the whole path of the file is not that useful, especially when the
> binary is from a build server where the path is something like
> /var/lib/jenkins/project-name/src/main/directory/foo.cpp.
>
> The current PR does this with some string manipulation at runtime, and
> is different from the const char* that is currently used, so it
> doesn't fit that well with the rest of the class. Since C++11 we can
> now do constexpr functions, so we should be able to do this at compile
> time(assuming you have optimizations turned on of course).
>
> We can do this one of several ways:
> 1. Take the PR as is(use strings and calculate at runtime)
> 2. Create a constexpr function that we control to calculate the
> filename at compile time as either an offset into the filename or a
> separate const char*. The advantage to this is that it doesn't
> require any other libraries for pre-C++17 compilers.
> 3. Use std::string_view(for C++17) or boost::string_view(pre-c++17).
> The following would work for this solution:
> std::string_view str{"/foo/bar/what.cpp"};
> const int location = str.find_last_of( '/' ) + 1;
> printf( "fullPath: %s\n", str.data() );
> printf( "fileName: %s\n", &str[location] );
>
> Does anybody have a preference or a better way to do it? I'm inclined
> to go with (3), since that does provide a good fallback for compilers
> that don't support C++17, and only requires boost for older compilers.
>
> -Robert Middleton
>