TDE
/
tqt3
8
0
Fork 0
Code Issues 23 Pull Requests 6 Releases Wiki Activity

Labels Milestones

Update tqt-mt.pc to export the same parameters previously exported in tqt.pc #200

Merged
MicheleC merged 1 commits from drop/tqtinterface into master 4 months ago
Conversation 1 Commits 1 Files Changed 5
MicheleC commented 4 months ago
Owner

Required to drop tqtinterface.

Required to drop tqtinterface.
MicheleC added this to the R14.2.0 release milestone 4 months ago
MicheleC added 1 commit 4 months ago
MicheleC requested review from Core 4 months ago
MicheleC requested review from Owners 4 months ago
MicheleC force-pushed drop/tqtinterface from 805696c12e to 48d6b0d862 4 months ago
obache reviewed 4 months ago
qmake/generators/unix/unixmake2.cpp
@ -1610,3 +1610,1 @@
t << "Libs: -L${libdir} -l" << lname.left(lname.length()-Option::libtool_ext.length()) << " ";
for(TQStringList::ConstIterator it = libs.begin(); it != libs.end(); ++it)
t << project->variables()[(*it)].join(" ") << " ";
t << "Libs: -L${libdir} -l" << lname.left(lname.length()-Option::libtool_ext.length()) << " -ltqui -L/usr/lib ";
obache commented 4 months ago
Collaborator

Why hard-coded "-L/usr/lib" here?

Why hard-coded "-L/usr/lib" here?
MicheleC commented 4 months ago
Poster
Owner

Good point, I have updated the PR

Good point, I have updated the PR
SlavekB commented 4 months ago
Owner

Here is the second thing to solve: Why is there forced linking of tqui library? The forced linking of this library was not part of tqt.pc, because it is a separate library that is definitely not needed for everyone. After all, that is why it is a separate library. This does not seem to be right to be forced to linked now.

Here is the second thing to solve: Why is there forced linking of `tqui` library? The forced linking of this library was not part of `tqt.pc`, because it is a separate library that is definitely not needed for everyone. After all, that is why it is a separate library. This does not seem to be right to be forced to linked now.
MicheleC commented 4 months ago
Poster
Owner

There are a few reason behing this choice.

  1. tqtinterface was providing two different .pc files (tqt.pc and tqtqui.pc) but only one shared object libtqt.so. This was already a design I didn't like.

  2. libtqt-mt.so and libtqui.so are both provided by tqt3 in the same package, so they will always be present together in the system

  3. while it is true that tqui is not always needed, providing it to the linker won't do much harm: if it is not needed, the linker will simply do nothing with it and the resulting executables/so files won't be any bigger

Given the above points, I saw an opportunity to simplify the detection code in cmake (no more need for detecting and linking tqui on its own) and that is why I implemented this way.

If we prefer to keep detection and linkage of tqui separate from that of tqt-mt, I will rework the code, but IMO it adds extra code for not real benefit.

There are a few reason behing this choice. 1. `tqtinterface` was providing two different `.pc` files (`tqt.pc` and `tqtqui.pc`) but only one shared object `libtqt.so`. This was already a design I didn't like. 2. `libtqt-mt.so` and `libtqui.so` are both provided by `tqt3` in the same package, so they will always be present together in the system 3. while it is true that `tqui` is not always needed, providing it to the linker won't do much harm: if it is not needed, the linker will simply do nothing with it and the resulting executables/so files won't be any bigger Given the above points, I saw an opportunity to simplify the detection code in cmake (no more need for detecting and linking `tqui` on its own) and that is why I implemented this way. If we prefer to keep detection and linkage of `tqui` separate from that of `tqt-mt`, I will rework the code, but IMO it adds extra code for not real benefit.
MicheleC commented 4 months ago
Poster
Owner

As per discussion on chat, we will keep single detection of tqt while linking tqui manually where needed. PR has been updated accordingly.

As per discussion on chat, we will keep single detection of tqt while linking `tqui` manually where needed. PR has been updated accordingly.
MicheleC marked this conversation as resolved
MicheleC force-pushed drop/tqtinterface from 48d6b0d862 to 85a5377226 4 months ago
MicheleC force-pushed drop/tqtinterface from 85a5377226 to 5b5cf7a81f 4 months ago
obache reviewed 4 months ago
qmake/generators/unix/unixmake2.cpp
@ -1609,4 +1609,1 @@
libs << "QMAKE_LFLAGS_THREAD"; //not sure about this one, but what about things like -pthread?
t << "Libs: -L${libdir} -l" << lname.left(lname.length()-Option::libtool_ext.length()) << " ";
for(TQStringList::ConstIterator it = libs.begin(); it != libs.end(); ++it)
t << project->variables()[(*it)].join(" ") << " ";
obache commented 4 months ago
Collaborator

Why other libs libraries are ignored? just redundant?

Why other `libs` libraries are ignored? just redundant?
MicheleC commented 4 months ago
Poster
Owner

They would cause FTBFS, for example -lgthread-2.0.
libtqt.pc provided by tqtinterface was exporting the same libraries that this PR is now exporting, so in terms of linkage for TDE applications, nothing has changed. Once you build a program using TQt, you need to link against tqt-mt. The extra libraries should probably used when building tqt (as needed) but there should be no need to expose them to all the programs using TQt. Perhaps a left over code from past.

They would cause FTBFS, for example `-lgthread-2.0`. `libtqt.pc` provided by `tqtinterface` was exporting the same libraries that this PR is now exporting, so in terms of linkage for TDE applications, nothing has changed. Once you build a program using TQt, you need to link against `tqt-mt`. The extra libraries should probably used when building `tqt` (as needed) but there should be no need to expose them to all the programs using TQt. Perhaps a left over code from past.
SlavekB reviewed 4 months ago
SlavekB left a comment
Owner

There are some comments.

There are some comments.
qmake/generators/unix/unixmake2.cpp
@ -1623,0 +1619,4 @@
// << varGlue("PRL_EXPORT_DEFINES","-D"," -D"," ")
// << project->variables()["PRL_EXPORT_CXXFLAGS"].join(" ")
// << varGlue("DEFINES","-D"," -D"," ")
<< " -DTQT_NO_ASCII_CAST -DTQT_NO_STL -DTQT_NO_COMPAT -DTQT_NO_TRANSLATION -DTQT_THREAD_SUPPORT -D_REENTRANT -I${includedir}";
SlavekB commented 4 months ago
Owner

I have tested that it does not have an impact on ABI, but it is strange that there are excluded definitions determined by configuration – like QT_SHARED, TQT_NO_DEBUG. And I hesitate whether it is right or not.

BTW, one space is at the end of "Cflags: ", second at the beginning " -DTQT_NO_ASCII_CAST..., so there is a doubled space.

I have tested that it does not have an impact on ABI, but it is strange that there are excluded definitions determined by configuration – like `QT_SHARED`, `TQT_NO_DEBUG`. And I hesitate whether it is right or not. BTW, one space is at the end of `"Cflags: "`, second at the beginning `" -DTQT_NO_ASCII_CAST...`, so there is a doubled space.
MicheleC commented 4 months ago
Poster
Owner

For QT_SHARED, TQT_NO_DEBUG, I looked into build logs and tried to mirror the exact variables as before. I had initially included these variables, then troubleshooting a FTBFS on a package I realized they were not used in the old build, so I removed them.

As for the double space, I will fix that.

For `QT_SHARED`, `TQT_NO_DEBUG`, I looked into build logs and tried to mirror the exact variables as before. I had initially included these variables, then troubleshooting a FTBFS on a package I realized they were not used in the old build, so I removed them. As for the double space, I will fix that.
MicheleC marked this conversation as resolved
src/kernel/tqt.h
@ -9,0 +61,4 @@
#include <tqdom.h>
#include "tqobject.h"
#include <tqdrawutil.h>
#include "tqbrush.h"
SlavekB commented 4 months ago
Owner

Is there any sense that the use of includes with quotes and sharp parentheses is mixed? I suppose it makes sense to use only sharp parentheses.

Is there any sense that the use of includes with quotes and sharp parentheses is mixed? I suppose it makes sense to use only sharp parentheses.
MicheleC marked this conversation as resolved
src/kernel/tqt.h
@ -14,0 +274,4 @@
#if defined( QT_MOC_CPP ) || defined( QT_H_CPP ) || defined( Q_OS_MACX )
#include <private/tqcom_p.h>
#include <private/tqucom_p.h>
#include "private/tqcom_p.h"
SlavekB commented 4 months ago
Owner

Two lines higher is the same include in a variant with sharp parentheses.

Two lines higher is the same include in a variant with sharp parentheses.
MicheleC marked this conversation as resolved
src/kernel/tqt.h
@ -14,0 +293,4 @@
#include "private/tqtextcodecinterface_p.h"
#include "private/tqpsprinter_p.h"
#include "private/tqtitlebar_p.h"
#include "private/tqucom_p.h"
SlavekB commented 4 months ago
Owner

A few dozen lines higher is the same include in a variant with sharp parentheses.

A few dozen lines higher is the same include in a variant with sharp parentheses.
MicheleC marked this conversation as resolved
MicheleC force-pushed drop/tqtinterface from 5b5cf7a81f to b6f43f9a98 4 months ago
MicheleC commented 4 months ago
Poster
Owner

Other that the point about defined variables, the rest has been updated.

Other that the point about defined variables, the rest has been updated.
SlavekB approved these changes 4 months ago
SlavekB left a comment
Owner

All seems good.

All seems good.
MicheleC force-pushed drop/tqtinterface from b6f43f9a98 to c489c62c17 4 months ago
MicheleC merged commit c489c62c17 into master 4 months ago
MicheleC deleted branch drop/tqtinterface 4 months ago

Reviewers

SlavekB approved these changes 4 months ago
TDE/Core was requested for review 4 months ago
The pull request has been merged as c489c62c17.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
SlavekB
TDE/Core
Labels
Apply labels
Clear labels   
GE/need-info

General - need additional info from contributor   
PR/keep-branch

Pull request - do not delete branch after merging   
PR/not-ok

Pull request - need fixing   
PR/rfc

Pull request - request for comments   
PR/update-trans

Pull request - update to translation files needed   
PR/wip

Pull request - work in progress   
RS/R14.0.x

Related to R14.0.x series   
RS/R14.1.x

Related to R14.1.x series   
SL/critical

Severity level - critical   
SL/major

Severity level - major   
SL/minor

Severity level - minor   
SL/normal

Severity level - normal   
SL/regression

Severity level - regression from previous version   
SL/trivial

Severity level - trivial   
SL/wishlist

Severity level - wishlist request   
ST/duplicate

Status - duplicate of another issue   
ST/invalid

Status - invalid report   
ST/notourproblem

Status - not our problem   
ST/rejected

Status - rejected   
ST/wontfix

Status - won't fix   
ST/worksforme

Status - works for me, unable to reproduce
No Label
GE/need-info
PR/keep-branch
PR/not-ok
PR/rfc
PR/update-trans
PR/wip
RS/R14.0.x
RS/R14.1.x
SL/critical
SL/major
SL/minor
SL/normal
SL/regression
SL/trivial
SL/wishlist
ST/duplicate
ST/invalid
ST/notourproblem
ST/rejected
ST/wontfix
ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.2.0 release
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Blocks
#383 DEB: drop tqtinterface module
TDE/tde-packaging
#37 Don't explicetly include tqt.h anymore
TDE/tde-common-admin
#120 Detects tqt-mt instead of tqt
TDE/tde-cmake
#74 Drop tqtinterface. Leave a short README for info only
TDE/tqtinterface
#7 Don't explicetly include tqt.h anymore
TDE/dbus-tqt
#28 Don't explicetly include tqt.h anymore
TDE/sip4-tqt
#315 Don't explicetly include tqt.h anymore
TDE/tdelibs
#545 Don't explicetly include tqt.h anymore
TDE/tdebase
#137 Don't explicetly include tqt.h and don't look for libtqui anymore.
TDE/tdepim
#51 Don't look for libtqui anymore.
TDE/tdesdk
#56 Don't explicetly include tqt.h anymore
TDE/tdevelop
#70 Don't explicetly include tqt.h anymore
TDE/amarok
#81 Use tqt-mt instead of tqt library
TDE/koffice
#26 Don't explicetly include tqt.h anymore
TDE/kvirc
#25 Don't explicetly include tqt.h anymore
TDE/rosegarden
#35 DEB compiz-trinity: adjust after dropping tqtinterface
TDE/extra-dependencies
Reference: TDE/tqt3#200
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'drop/tqtinterface'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes