TDE
/
tqt3
9
0
Fork 0
Code Issues 20 Pull Requests 7 Releases Wiki Activity
Labels Milestones

Add hidden visibility for linux-g++-64 mkspecs. #44

Merged
MicheleC merged 1 commits from fix/hidden_visibility_linux-g++-64 into master 4 years ago
Conversation 6 Commits 1 Files Changed 2
Ghost commented 4 years ago

Add new paths for X11 includes and libraries.
Some cosmetics.

For those who want to build with:

./configure -platform "linux-g++-64"

This shouldn't be explicitely requested E.G: https://mirror.git.trinitydesktop.org/gitea/TDE/tde-packaging/src/branch/master/redhat/dependencies/tqt3/tqt3.spec#L1285

Add new paths for X11 includes and libraries. Some cosmetics. For those who want to build with: ``` ./configure -platform "linux-g++-64" ``` This shouldn't be explicitely requested E.G: https://mirror.git.trinitydesktop.org/gitea/TDE/tde-packaging/src/branch/master/redhat/dependencies/tqt3/tqt3.spec#L1285
Ghost commented 4 years ago
Poster

I kept the former path for X11 (aka /usr/X11R6/include and /usr/X11R6/lib ), I'm not sure It's still relevant these days.

I kept the former path for X11 (aka **/usr/X11R6/include** and **/usr/X11R6/lib** ), I'm not sure It's still relevant these days.
SlavekB commented 4 years ago
Owner

I'm surprised that hidden visibility isn't the default for linux-g++-64. So that's a good idea. It seems the same could be the case for linux-g++-32.

I'm surprised that hidden visibility isn't the default for linux-g++-64. So that's a good idea. It seems the same could be the case for linux-g++-32.
SlavekB commented 4 years ago
Owner

I kept the former path for X11 (aka /usr/X11R6/include and /usr/X11R6/lib ), I'm not sure It's still relevant these days.

I don't think these paths are currently valid for any Linux.

> I kept the former path for X11 (aka **/usr/X11R6/include** and **/usr/X11R6/lib** ), I'm not sure It's still relevant these days. I don't think these paths are currently valid for any Linux.
Ghost commented 4 years ago
Poster

I'm surprised that hidden visibility isn't the default for linux-g++-64. So that's a good idea. It seems the same could be the case for linux-g++-32.

ok, so I'll amend this PR accordingly for tomorrow (or later).

> I'm surprised that hidden visibility isn't the default for linux-g++-64. So that's a good idea. It seems the same could be the case for linux-g++-32. ok, so I'll amend this PR accordingly for tomorrow (or later).
Ghost added the PR/wip label 4 years ago
Ghost changed title from Add hidden visibility for linux-g++-64 mkspecs. to WIP: Add hidden visibility for linux-g++-64 mkspecs. 4 years ago
Ghost commented 4 years ago
Poster

linux-g++-32 has been adjusted for hidden symbols visibility but not tested.

linux-g++-32 has been adjusted for hidden symbols visibility but not tested.
Ghost removed the PR/wip label 4 years ago
Ghost changed title from WIP: Add hidden visibility for linux-g++-64 mkspecs. to Add hidden visibility for linux-g++-64 mkspecs. 4 years ago
MicheleC requested changes 4 years ago
MicheleC left a comment
Owner

The changes looks good, but the loss of alignment on the 64 bit version is not a good idea in my opinion.

First of all the file is much more readable when all the options are nicely aligned. Secondly checking at random qmake.conf in some of the other folders in mkspecs, they are all nicely aligned. So if we want to change the alignment, we should change it for all of them, not just a single one.

The changes looks good, but the loss of alignment on the 64 bit version is not a good idea in my opinion.<br/> First of all the file is much more readable when all the options are nicely aligned. Secondly checking at random qmake.conf in some of the other folders in mkspecs, they are all nicely aligned. So if we want to change the alignment, we should change it for all of them, not just a single one.
Ghost commented 4 years ago
Poster

Brought back the former alignment, my IDE's indentation was not right.

Brought back the former alignment, my IDE's indentation was not right.
SlavekB requested review from MicheleC 4 years ago
SlavekB approved these changes 4 years ago
SlavekB left a comment
Owner

It looks good. It seems necessary to do a rebase to the current HEAD.

It looks good. It seems necessary to do a rebase to the current HEAD.
MicheleC requested changes 4 years ago
MicheleC left a comment
Owner

Thanks for fixing up the mis-alignment in the 6bit code. See further comment below about include/lib folders. We shall discuss further before we merge this PR, IMO.

Thanks for fixing up the mis-alignment in the 6bit code. See further comment below about include/lib folders. We shall discuss further before we merge this PR, IMO.
mkspecs/linux-g++-64/qmake.conf Outdated
QMAKE_LIBDIR =
QMAKE_INCDIR_X11 = /usr/X11R6/include
QMAKE_LIBDIR_X11 = /usr/X11R6/lib64
QMAKE_INCDIR_X11 = /usr/X11R6/include /usr/include /usr/include/X11
MicheleC commented 4 years ago
Owner

I am wondering why in the 64 bit version you have changed these definitions but not on the 32 bit version. It seems weird to me.

The change is partially good in my opinion: I would move /usr/include/X11 to the beginning so it would be the first folder while I wonder if addind /usr/include to a QMAKE_INCDIR_X11 is appropriate or not, given the variable is specifically for X11.

Once we decide on what we want to do, I think we should probably do the same changes to the other folders too. What do you think?

I am wondering why in the 64 bit version you have changed these definitions but not on the 32 bit version. It seems weird to me. The change is partially good in my opinion: I would move /usr/include/X11 to the beginning so it would be the first folder while I wonder if addind /usr/include to a QMAKE_INCDIR_X11 is appropriate or not, given the variable is specifically for X11. Once we decide on what we want to do, I think we should probably do the same changes to the other folders too. What do you think?
SlavekB commented 4 years ago
Owner

Basically, /usr/include is the default include path, which does not need to be specified. When calling pkg-config, the behavior is similar – if include is the default /usr/include, it is omitted. So it is likely that /usr/include may also be omitted here.

Basically, `/usr/include` is the default include path, which does not need to be specified. When calling pkg-config, the behavior is similar – if include is the default `/usr/include`, it is omitted. So it is likely that `/usr/include` may also be omitted here.
MicheleC commented 4 years ago
Owner

Exactly.

I think we should limit this PR to the hidden visibility changes only. Then if we want to review the various inlcude/lib folders, we should do that separately (new PR) and for all archs/compilers, for consistency.

Exactly. I think we should limit this PR to the hidden visibility changes only. Then if we want to review the various inlcude/lib folders, we should do that separately (new PR) and for all archs/compilers, for consistency.
MicheleC approved these changes 4 years ago
MicheleC left a comment
Owner

Looks good 👍

Thanks for the various changes.

Looks good :+1:<br/> Thanks for the various changes.
MicheleC merged commit 9afb3f4d91 into master 4 years ago
MicheleC deleted branch fix/hidden_visibility_linux-g++-64 4 years ago
MicheleC added this to the R14.0.9 release milestone 4 years ago

Reviewers

SlavekB approved these changes 4 years ago
MicheleC approved these changes 4 years ago
The pull request has been merged as 9afb3f4d91.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
SlavekB
MicheleC
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.0.9 release
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tqt3#44
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'fix/hidden_visibility_linux-g++-64'

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

No
Yes