search

jarvis657 / snappy

Automatically exported from code.google.com/p/snappy
Other
0 stars 0 forks source link

snappy.cc includes static initializer #60

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
We attempted to roll the latest version of snappy into Chromium and enable it 
for leveldb. The build infrastructure reported a new static initializer being 
introduced so we had to revert.

The report is:

# snappy.ccstd::ios_base::Init::Init()@plt
# snappy.ccstd::__ioinit [#includes <iostream>, use <ostream> instead]
# snappy.cc__cxa_atexit@plt [registers a dtor to run at exit]

It looks like this is coming from snappy-stubs-internal.h

Is it possible to follow the guidance and just use ostream instead?

Original issue reported on code.google.com by jsb...@google.com on 5 Apr 2012 at 6:44

GoogleCodeExporter commented 9 years ago 
Looks like the logging mechanism is hardcoded to use cerr, which would need to 
change in some way.

Original comment by jsb...@google.com on 5 Apr 2012 at 8:36

GoogleCodeExporter commented 9 years ago 
Hmm, what version did you try to upgrade from? We haven't changed the logging 
since initial release, so this has to be something accidential.

Original comment by se...@google.com on 11 Apr 2012 at 10:33

GoogleCodeExporter commented 9 years ago 
Snappy was not previously enabled for leveldb in Chromium, and so was not being 
linked in, so nothing tripped the static initializer warning.

My comment about "roll the latest..." is not relevant.

Original comment by jsbell@chromium.org on 11 Apr 2012 at 2:22

GoogleCodeExporter commented 9 years ago 
For clarity, let me restate:

When trying out enabling snappy compression for leveldb in chromium, our build 
infrastructure detected the use of static initializers, so the change was 
reverted.

On the chromium side we could suppress the static initializer alerts in snappy 
as a third party library - we do that for a small handful of other libraries. 
We would prefer not to, and we suspect that since snappy is being used in many 
other projects they would benefit from removing the static initializers, 
especially as they appear to only be used for logging.

One option might be to refactor the logging into snappy-stubs-public.h which 
the containing project provides, and allow the project to specify its own 
mechanism.

Original comment by jsbell@chromium.org on 11 Apr 2012 at 3:51

GoogleCodeExporter commented 9 years ago 
OK. But what should we do on e.g. CHECK-fail then? (That's really the only 
place where we log.) We'd have to output the log to somewhere...

I understand that we can make a modular API, but given that this needs to be 
used within macros, it sounds like a recipe for quite a lot of complexity.

Original comment by se...@google.com on 11 Apr 2012 at 4:58

GoogleCodeExporter commented 9 years ago 
In the case of Chromium, we have equivalent LOG/VLOG macros already. For 
leveldb these get used in the "port": 
http://src.chromium.org/viewvc/chrome/trunk/src/third_party/leveldatabase/env_ch
romium.cc?view=markup - but leveldb already has a modular "env" concept.

For snappy, it looks like we could include these via snappy-stubs-public.h and 
snappy-stubs-internal.h could define LOG and VLOG if they aren't already 
defined, pulling in <iostream> conditionally. CRASH_UNLESS could be changed to 
use LOG(ERROR).

If that seems like a possibility I'll mock it up.

Original comment by jsbell@chromium.org on 11 Apr 2012 at 6:15

GoogleCodeExporter commented 9 years ago 
Given that LevelDB is one of our major open-source customers, I'd say that 
sounds reasonable. Make a mock and I'll have a look?

Original comment by se...@google.com on 12 Apr 2012 at 9:15

GoogleCodeExporter commented 9 years ago 
Hi,

Any updates here?

Original comment by sgunder...@bigfoot.com on 20 Apr 2012 at 9:31

GoogleCodeExporter commented 9 years ago 
I'm hoping to get to it next week.

Original comment by jsbell@chromium.org on 20 Apr 2012 at 5:07

GoogleCodeExporter commented 9 years ago 
Just a quick status update:

With minimal effort I have Chromium building against the attached patch, which 
lets a port (via snappy-stubs-public.h) #define PORT_LOGGING_AND_CHECKING if it 
provides equivalent LOG/VLOG/CHECK*/DCHECK* macros. 

That #define name is pretty hideous so I welcome a better suggestion, and I'd 
like to do more testing and get a review on the Chromium side before saying 
"yes, this it the best approach".

Original comment by jsbell@chromium.org on 26 Apr 2012 at 11:15

Attachments:

GoogleCodeExporter commented 9 years ago 
That's certainly non-intrusive, at least...

What about renaming it to USE_EXTERNAL_LOGGING?

Original comment by se...@google.com on 27 Apr 2012 at 8:47

GoogleCodeExporter commented 9 years ago 
Agreed, that's a much better name.

With the attached snappy patch (same as above with the #define renamed), 
Chromium builds with it locally with this change: 
http://codereview.chromium.org/9866056/

If this approach still seems reasonable, we can move forward on the Chromium 
side once this patch is included.

Original comment by jsb...@google.com on 27 Apr 2012 at 9:39

Attachments:

GoogleCodeExporter commented 9 years ago 
I had to change it a bit, since your patch pulls #include <iostream> within the 
“namespace snappy {” declaration. I've sent you a code review; please take 
a look. :-)

Original comment by se...@google.com on 30 Apr 2012 at 10:24

GoogleCodeExporter commented 9 years ago 
Fixed in r62.

Original comment by se...@google.com on 22 May 2012 at 9:33