Do you think this change is valid for release 2.2.0?
I mean modules that depend on openmrs-core just need to ensure that they use the facade slf4j and not the implementations commons-logging and log4j (like we will do in the core as well lets clean up logging).
I would very much welcome that we replace a very outdated library and would also volunteer to work on this in openmrs-core and help modules to make the change But this will probably affect all modules.
I mean every module
will probably have a api/src/test/resources/log4j.xml which needs to be replaced with a log4j2.xml (of course we can provide a working sample)
needs to replace all its uses of log4j and apache commons with the facade of slf4j (also an easy task but still work
It would be great to use this opportunity to rethink how modules configure logging. Including log4j.xml is a bad practice as it may mess up/overwrite the global configuration and it introduces a dependency on the log4j version as we just learned the hard way…
One problem that always comes up with this–is it possible to make modules compatible with both the “before” and “after” of this change without forking the module? We generally do our utmost to avoid changes that would require forking modules to maintain compatibility with different versions of Core.
If every module makes sure that they only use the slf4j API to log messages than it would not need to worry about what logging implementation openmrs-core uses as implementation. So no compatibility changes from before/after. The slf4j facade lib is already in openmrs-core. I dont know why it is not used exclusively (see clean up logging).
slf4j does not support the log level FATAL. So it does not provide log.fatal() and thus such calls have to be changed into info.error("message", exception) or using markers info.error(MarkerFactory.getMarker("FATAL"), "message", exception)
We really should only be using logging for the basic info/debug/etc. I suspect as we start testing against modules we may find some bad practices that may need to be cleaned up, but from what you say, it sounds more like it will be tedious rather than conceptually difficult. And it sounds like after any cleanup, it should work fine before/after which is great.
So, thanks, I don’t have major concerns then.
I’d suggest we start testing with some of the core modules bundled with the reference app and see if we come into any unexpected issues.
IMHO yes, if it breaks compatibility (for a good cause) the modules affected should be migrated to be in synch with core 2.2. Or else they can stay unmodified as long as they run with core < 2.2. I guess it’s a matter of lack of resources, if there was a team responsible of each module the migration could be done in parallel in a few days.
I’m not so much concerned with the migration effort (though that does need to be considered). But having to maintain two branches of modules, one compatible with < 2.2 and one 2.2+ is something I’d be very reluctant to do. However, it sounds like @teleivo thinks this won’t be an issue.
But we already have that right? I assume release branches of modules only work with certains version of core, not with the latest. Also the branching model we use is trunk-based, and only important fixes are backported by cherry picking, not new developments or refactorings. Maybe I’m wrong.
Are the needed changes predictable enough that we could provide a script to convert the module? When we switched to using Maven, Ben made a shell script that converted a standard module layout to the new maven organization, added the pom, etc. That made it much easier for modules to migrate.
Could we back port support for slf4j? Or maybe make a separate module that injects slf4j support in oldest systems such that other modules could use it. This way, a module migrated to use slf4j could still run on older versions.
Thats already taken care of by the logging facade slf4j which is in core and everyone (core and modules) should use it instead of the logging implementation (be it log4j version 1 or 2).
I’m not saying to remove slf4j, I am saying we should use it There is no need to inject its already in core a long time.
So as first step modules have to:
Replace their calls to commons-logging or log4j version 1 with calls to slf4j, so they are not tied to the logging implementation openmrs uses! Sample commit for the radiology module here. This can be done today on any branch. The only consequence when doing this switch is that:
slf4j does not support the log level FATAL. So it does not provide log.fatal() and thus such calls have to be changed into info.error(“message”, exception) or using markers info.error(MarkerFactory.getMarker(“FATAL”), “message”, exception)
I can think of a script/IDE automation to support module devs and am volunteering to do this for a handful.
As a background info for those who havent read it lets clean up logging
This step is independent of switching logging implementations and should be done regardless of whether we switch and when, it can be done ASAP.
As a second step modules have to:
Exchange their log4j.xml file that they include in the src/test/resources for testing purposes with a log4j2-text.xml. We will of course supply the file.
My 2 cents: We use log4j2 on several projects where I work and it is great. It performs very well, has lots of features, is quite flexible in both API and configuration. I suggest log4j2 and nothing else (i.e. slf4j).
As a side note @lluismf generally the master branches of modules work with much earlier versions of core. We are running, more or less, the latest releases (or even snapshots) of most of the reference app modules, plus some other modules, with OpenMRS 1.10.x. It’s rare that we have branches of modules targeted to certain versions of core. There’s limitations caused by maintaining this, of course, and there’s a chance we may have to change that in the future, but it would require some thought.