jduquennoy / Log4swift

A logging framework for the swift language
Apache License 2.0
43 stars 9 forks source link

XCode 11.4 - Crash after `objc_copyClassList` call #25

Open serges147 opened 4 years ago

jduquennoy commented 4 years ago

Indeed, I could reproduce the crash running the unit tests. I will investigate that problem and fix it asap, thanks for reporting !

jduquennoy commented 4 years ago

I pushed a commit that fixes the crash. It did involve a change though, that might be breaking for people using custom appenders, and references them from a config file: Previously, the different appenders subclasses were auto-discovered by getting a list of the classes from the runtime. This was the part that was crashing. I changed it to a static list of types, with the ability to register other types using AppendersRegistry.registerAppender(Appender.Type). So now, to use a custom appender in configuration files, this method should be called before trying to load the config file.

I still need to work out some problems with the unit tests though: all tests for the AppleUnifiedLoggerAppender are failing, as the reading of the logged messages fails.

serges147 commented 4 years ago

I've just build everything with Log4Swift pod targeted to this commit. It works like a charm! And we do have custom appender - it was matter of 1 line change to bring it back (using new registerAppender). Thank You!

serges147 commented 4 years ago

https://developer.apple.com/documentation/xcode_release_notes/xcode_11_4_1_release_notes

Swift Resolved Issues Fixed a crash that could occur in Swift code that imported an Objective-C class defined with the objc_runtime_name attribute. (60888835)

serges147 commented 4 years ago

FYI: Looks like there is some solution here: https://bugs.swift.org/browse/SR-12344

serges147 commented 4 years ago

@jduquennoy Hi! Could you please take a look on my draft PR: https://github.com/jduquennoy/Log4swift/pull/26 I believe that with this fix we don't need AppendersRegistry (or we can do hybrid approach - support "old" way and the new "registry" one).

I tried to compile and run unit tests on xcode 10.3, 11.3.1 and 11.5 - all works ok (no crash, tests are green). Also, I fixed couple of warnings (indexfirstIndex) which appear when Swift 5 in use.

I would like to address this issue rather sooner than later - it blocks us to switch to xcode 11.5 (we stuck on 11.3 currently b/c of it). Please let me know what else can I do to help? Thanks!

jduquennoy commented 4 years ago

Hi Sergei,

Thanks millions for this PR ! I'm quite out of time today, i will take a look at your PR tomorrow, beginning of the day. Sorry for the delay :-(

Jérôme

serges147 commented 4 years ago

Hey @jduquennoy ! Please take a look for unit tests fix PR: https://github.com/jduquennoy/Log4swift/pull/27 Thanks!

serges147 commented 4 years ago

@jduquennoy So, it looks like the last thing which should be done is actually implement the "hybrid" approach, namely: clients could use either "old" automatic way of getting appenders, or register an appender explicitly via new api. In such way, I believe, it won't be breaking change, and we can release 1.1.0 (as minor update). Jacques, what do you think will the right way of doing it? Is it almost as just returning back the objc_copyClassList call? Will it correctly coexist with the new approach? Thanks!

serges147 commented 4 years ago

@jduquennoy Gentle ping about 👆 BTW, I'm gonna prepare additional PR - fix for iOS (AppleUnifiedLoggerAppender).

serges147 commented 4 years ago

@jduquennoy And here is the PR about 👆 Please take a look. Thanks https://github.com/jduquennoy/Log4swift/pull/28

serges147 commented 4 years ago

@jduquennoy Hello, any chance to release 1.1.0 to the Cocoapods (see my comment on Jun 13)? Thanks!

jduquennoy commented 4 years ago

Hi Sergei, I tagged pushed the head of Master to cocoapods, as v1.1.0. Sadly, I still have tests failing on Travis, for Apple Unified Logs. I have to rework those tests, to capture the logs in a live stream instead of trying to get them after the tests ran (it is the part that fails in the tests, even if the manual tests I have done shows that it does the logs are issued).

serges147 commented 4 years ago

Thanks @jduquennoy !