#332 Bundling exception for IQmol
Closed: Fixed None Opened 5 years ago by jussilehtola.

Hi,

related to https://bugzilla.redhat.com/show_bug.cgi?id=984175, IQmol bundles two external projects: qmsgbox [1] and QstLog [2]. Both are very small projects, and the upstreams do not support building into libraries. I'd like to get an exception for these.

[1] http://www.qtcentre.org/wiki/index.php?title=QMsgBox_%28Solves_the_QMessageBox_icon_problem%29
[2] https://bitbucket.org/razvanpetru/qt-components/wiki/QsLog


Hi,

I've had a look at the source code and I can't find any good technical reason of why those libraries has been bundled. There actually is no changes made to them since initial import into his/her git repo.

Those projects should first be packaged even if they are "small project" and skipped just like you did with GL2PS.

So, this a -1 from me.

FPC talked about this and without more information, agrees with laxathom's evaluation. The libraries are old but they do have separate upstreams. The upstreams seem to have build scripts that create library files. So they should be shipped as separate packages.

Replying to [comment:2 toshio]:

The upstreams seem to have build scripts that create library files.

Incorrect. There are no library targets. In both cases the build script just compiles the test program.

Replying to [comment:3 jussilehtola]:

Incorrect. There are no library targets. In both cases the build script just compiles the test program.
They are the typical underdeveloped qmake based stuff, which lacks all build/install infrastructure.

They can fairly easily be converted into libs.

That's a bit beside the point, isn't it?

For any project it's fairly easy to come up with makefiles that generate libraries.

Replying to [comment:5 jussilehtola]:

That's a bit beside the point, isn't it?
No. What I am actually saying is these upstream do a poor job. If we want to use their stuff, we need to work on it, or we're better off not to ship this stuff at all.

I'm more on the fence about this. Some meeting logs:
{{{
<abadger1999> Hmm... Looks like they're code, don't have any makefiles or other build scrupts at all.
<abadger1999> But it doesn't look like they're intended to be modified per se.
<abadger1999> otoh, I don't think gnulib or egglib are intended to be modified.
<smoother1rogz> abadger1999: they do have *.pro file which build Makefile
<abadger1999> ah
<limburgher> So it's less like they're copylibs and more like they're just poorly released? Or are they more like some of these head-only packages?
<abadger1999> Smoother1rOgZ: What do you get out of running make ? Are they .o files or a .so?
<smoother1rogz> you got both at the end
}}}

If the .pro files don't, in fact, generate Makefiles that build .so then I'd be inclined to reexamine whether these fall under the definition of copylibs.

Before we make a final decision on this, we'd like for you to reach out to the upstreams for these two components and ask them if they would be willing to apply a change to their source code to generate libraries (preferrably shared, but static would be okay too). Please let us know how they answer (or if they do not answer in a month).

laxathom was willing to make patches for this, i'll let him post links when he does.

Well, QMsgBox now builds a shared library. Still waiting to hear about QstLog.

Well, QMsgBox now builds a shared library. Still waiting to hear about !QstLog.
Correct, saw it in upstream branch, thanks!

In the mean time, here's the patch to build !QstLog as shared or static library.[[BR]]
http://laxathom.fedorapeople.org/infra-fp/fpc/IQmol

Got a reply from !QsLog upstream

"!QsLog was designed to be included in a project, but I can see how having a library can also be useful.
I will integrate the patch hopefully this week, or next week at the latest. Right now it doesn't seem to be cross-platform, so I will have to change it for all the !QsLog supported OSes."

so he thinks !QsLog is a copylib. I'll keep this open until !QsLog can be packaged.

Ah, excellent!

I can for sure make it cross-platform even though it builds and works just fine on arm, mips & intelce.

Hi,

Commit [1] adds a separate .pro file which builds QsLog as a shared library and commit [2] credits laxathom.

I will test this on Linux tomorrow, but I would appreciate it if you could take a look and see if anything is obviously wrong. On Mac it outputs the dylib as expected.
The implementation is not yet complete, I also have to make sure that the correct functions and classes will be exported from the DLL on Windows.

[1] https://bitbucket.org/razvanpetru/qt-components/commits/2e7f9184c2cba3f62f0834939049865c95504a5e

[2] https://bitbucket.org/razvanpetru/qt-components/commits/9f7dd2171f525d3ec2e903dd0d34240aaabb1baa

Well, IQmol doesn't compile against the trunk release of QsLog.

Could you please tell me what kind of errors are you getting?

main.C: In function 'int main(int, char**)':
main.C:59:46: error: 'QsLogging::DestinationPtr' has no member named 'get'
logger.addDestination(fileDestination.get());
^

This is because the type of smart pointer behind the DestinationPtr has been changed. Previously they were auto_ptr, now they are QSharedPointer.

The reason for the change is that some users of the library were creating destinations inside an if block and the auto_ptrs were destroyed after they were added to the log, leading to crashes.

Removing .get() and simply passing the variable should fix the error. Is this acceptable for you?

Well, that fixed that, then the next errors.

LogMessageDialog.C: In member function 'QsLogging::Level IQmol::LogMessageDialog::messageLevel(const QString&)':
LogMessageDialog.C:107:23: error: 'TraceString' is not a member of 'QsLogging'
if (msg.startsWith(QsLogging::TraceString)) {
^
LogMessageDialog.C:109:30: error: 'DebugString' is not a member of 'QsLogging'
} else if (msg.startsWith(QsLogging::DebugString)) {
^
LogMessageDialog.C:111:30: error: 'InfoString' is not a member of 'QsLogging'
} else if (msg.startsWith(QsLogging::InfoString)) {
^
LogMessageDialog.C:113:30: error: 'WarnString' is not a member of 'QsLogging'
} else if (msg.startsWith(QsLogging::WarnString)) {
^
LogMessageDialog.C:115:30: error: 'ErrorString' is not a member of 'QsLogging'
} else if (msg.startsWith(QsLogging::ErrorString)) {
^
LogMessageDialog.C:117:30: error: 'FatalString' is not a member of 'QsLogging'
} else if (msg.startsWith(QsLogging::FatalString)) {

Interesting, those variables were originally not intended to be used externally so I converted them to static globals from static class members.

What I could do is add a public static function that tries to parse the message level from a string - basically an official version of your function. I will work on that this evening unless something unexpected comes up.

Commit 1 contains the function levelFromLogMessage, and then you can switch on the return value if you want to do specific processing based on the level.

From my understanding of the above comments, all of the original issues that led to this exception request have been resolved and thus this ticket is no longer relevant. However, it's completely possible that I'm totally wrong, so feel free to reopen this.

Login to comment on this ticket.

Metadata