TDE
/
tdebase
9
4
Fork 1
Code Issues 116 Pull Requests 10 Releases Wiki Activity
Labels Milestones

feat/ask-pkgconfig-for-kb-rules #158

Merged
SlavekB merged 3 commits from feat/ask-pkgconfig-for-kb-rules into master 4 years ago
Conversation 10 Commits 3 Files Changed 4
aneejit1 commented 4 years ago
Collaborator

kxkb tries to guess where to find the xkb rules file, but if it is not found in one of the listed hard coded locations, the only choice presented is the US keyboard layout. Try to obtain an accurate location at configuration time for the directory from several sources that may be available, and allow an override to be specified. Some additional possible locations have also been added to xkbd's hardcoded list for the runtime tests.

The tests for libICE and libXss have also been updated to allow paths provided by pkg-config to be checked.

kxkb tries to guess where to find the xkb rules file, but if it is not found in one of the listed hard coded locations, the only choice presented is the US keyboard layout. Try to obtain an accurate location at configuration time for the directory from several sources that may be available, and allow an override to be specified. Some additional possible locations have also been added to xkbd's hardcoded list for the runtime tests. The tests for libICE and libXss have also been updated to allow paths provided by pkg-config to be checked.
MicheleC commented 4 years ago
Owner

This looks and work good. I manually tried all 4 scenarios and got the correct location each time.

@aneejit1 could you squash the commits into one since it is all related?

@SlavekB could you test on older distros?

This looks and work good. I manually tried all 4 scenarios and got the correct location each time. @aneejit1 could you squash the commits into one since it is all related? @SlavekB could you test on older distros?
MicheleC requested review from SlavekB 4 years ago
aneejit1 commented 4 years ago
Poster
Collaborator

The xkbd commits have now been squished into one.

The xkbd commits have now been squished into one.
MicheleC commented 4 years ago
Owner

The xkbd commits have now been squished into one.

Thanks, although it would have been better to avoid all the duplicated "Signed-off-by: aneejit1 aneejit1@gmail.com" and just have one on that commit.

Since your commits don't have a GPG signature, we can do that before merging the code.

Now just waiting for Slavek's opinion and testing on older distros.

> The xkbd commits have now been squished into one. Thanks, although it would have been better to avoid all the duplicated "Signed-off-by: aneejit1 <aneejit1@gmail.com>" and just have one on that commit.<br/> Since your commits don't have a GPG signature, we can do that before merging the code. Now just waiting for Slavek's opinion and testing on older distros.
SlavekB requested changes 4 years ago
SlavekB left a comment
Owner

Some simplifications can be made here in conditions and REGEX expressions.
See comments.

Some simplifications can be made here in conditions and REGEX expressions. See comments.
ConfigureChecks.cmake Outdated
ERROR_STRIP_TRAILING_WHITESPACE
RESULT_VARIABLE SETXKBMAP_RC
)
if( "${SETXKBMAP_RC}" STREQUAL "0" )
SlavekB commented 4 years ago
Owner

I think that a validating condition containing MATCHES could be used here, and consequently that all subsequent string( REGEX... ) could be replaced by one expression. The result could look like this:

    if( "${SETXKBMAP_RC}" STREQUAL "0" AND
        "${SETXKBMAP_OUTPUT}" MATCHES "\nFILES.*\n[^/]*(/[^\n]*/)xkb[ \t]*\n" )
      string( REGEX REPLACE ".*\nFILES.*\n[^/]*(/[^\n]*/)xkb[ \t]*\n.*" "\\1" SETXKBMAP_OUTPUT
"${SETXKBMAP_OUTPUT}" )
      if( EXISTS "${SETXKBMAP_OUTPUT}" )
        set( X11_XKB_RULES_DIR "${SETXKBMAP_OUTPUT}" )
      endif( )
    endif( )
I think that a validating condition containing `MATCHES` could be used here, and consequently that all subsequent `string( REGEX... )` could be replaced by one expression. The result could look like this: ``` if( "${SETXKBMAP_RC}" STREQUAL "0" AND "${SETXKBMAP_OUTPUT}" MATCHES "\nFILES.*\n[^/]*(/[^\n]*/)xkb[ \t]*\n" ) string( REGEX REPLACE ".*\nFILES.*\n[^/]*(/[^\n]*/)xkb[ \t]*\n.*" "\\1" SETXKBMAP_OUTPUT "${SETXKBMAP_OUTPUT}" ) if( EXISTS "${SETXKBMAP_OUTPUT}" ) set( X11_XKB_RULES_DIR "${SETXKBMAP_OUTPUT}" ) endif( ) endif( ) ```
aneejit1 commented 4 years ago
Poster
Collaborator

There are control codes in the output from "man", so that doesn't actually work. However, how about this instead...

if( "${SETXKBMAP_RC}" STREQUAL "0" AND
    "${SETXKBMAP_OUTPUT}" MATCHES "\n.*FILES.*\n[^/]*(/[^\n]*/xkb).*\n" )
  if( EXISTS "${CMAKE_MATCH_1}" )
    set( X11_XKB_RULES_DIR "${CMAKE_MATCH_1}" )
  endif( )
endif( )
There are control codes in the output from "man", so that doesn't actually work. However, how about this instead... if( "${SETXKBMAP_RC}" STREQUAL "0" AND "${SETXKBMAP_OUTPUT}" MATCHES "\n.*FILES.*\n[^/]*(/[^\n]*/xkb).*\n" ) if( EXISTS "${CMAKE_MATCH_1}" ) set( X11_XKB_RULES_DIR "${CMAKE_MATCH_1}" ) endif( ) endif( )
aneejit1 commented 4 years ago
Poster
Collaborator

Just realised that the "set" really ought to be

    string( REGEX REPLACE "/xkb$" "/" X11_XKB_RULES_DIR "${CMAKE_MATCH_1}" )

otherwise the "/kbd" at the end would cause kxbd to miss the directory.

Just realised that the "set" really ought to be string( REGEX REPLACE "/xkb$" "/" X11_XKB_RULES_DIR "${CMAKE_MATCH_1}" ) otherwise the "/kbd" at the end would cause kxbd to miss the directory.
SlavekB commented 4 years ago
Owner

Great, using CMAKE_MATCH_1 is a very good idea!

Here's another adjustment: Move the parentheses between / and kbd so that CMAKE_MATCH_1 contains the exact result you want and no subsequent replacement is needed:

if( "${SETXKBMAP_RC}" STREQUAL "0" AND
    "${SETXKBMAP_OUTPUT}" MATCHES "\n.*FILES.*\n[^/]*(/[^\n]*/)xkb.*\n" )
  if( EXISTS "${CMAKE_MATCH_1}" )
    set( X11_XKB_RULES_DIR "${CMAKE_MATCH_1}" )
  endif( )
endif( )
Great, using `CMAKE_MATCH_1` is a very good idea! Here's another adjustment: Move the parentheses between `/` and `kbd` so that `CMAKE_MATCH_1` contains the exact result you want and no subsequent replacement is needed: ``` if( "${SETXKBMAP_RC}" STREQUAL "0" AND "${SETXKBMAP_OUTPUT}" MATCHES "\n.*FILES.*\n[^/]*(/[^\n]*/)xkb.*\n" ) if( EXISTS "${CMAKE_MATCH_1}" ) set( X11_XKB_RULES_DIR "${CMAKE_MATCH_1}" ) endif( ) endif( ) ```
aneejit1 commented 4 years ago
Poster
Collaborator

The inclusion of the "/kbd" in the match is actually deliberate on my part and ties into the reason why the "EXISTS" check is present. Given that the "man" page contains control characters, there is the risk that those characters could wind up in the output string. There's also the possibility that another file/directory could be added to the "FILES" section that could alter the result. I can remove it in the "if" if you like, but I'd prefer to keep the "/kbd" as it effectively makes the "EXISTS" check more accurate.

The inclusion of the "/kbd" in the match is actually deliberate on my part and ties into the reason why the "EXISTS" check is present. Given that the "man" page contains control characters, there is the risk that those characters could wind up in the output string. There's also the possibility that another file/directory could be added to the "FILES" section that could alter the result. I can remove it in the "if" if you like, but I'd prefer to keep the "/kbd" as it effectively makes the "EXISTS" check more accurate.
aneejit1 commented 4 years ago
Poster
Collaborator

Ah! I've just realised the compromise solution!

if( "${SETXKBMAP_RC}" STREQUAL "0" AND
    "${SETXKBMAP_OUTPUT}" MATCHES "\n.*FILES.*\n[^/]*(/[^\n]*/)xkb.*\n" )
  if( EXISTS "${CMAKE_MATCH_1}xkb" )
    set( X11_XKB_RULES_DIR "${CMAKE_MATCH_1}" )
  endif( )
endif( )
Ah! I've just realised the compromise solution! if( "${SETXKBMAP_RC}" STREQUAL "0" AND "${SETXKBMAP_OUTPUT}" MATCHES "\n.*FILES.*\n[^/]*(/[^\n]*/)xkb.*\n" ) if( EXISTS "${CMAKE_MATCH_1}xkb" ) set( X11_XKB_RULES_DIR "${CMAKE_MATCH_1}" ) endif( ) endif( )
SlavekB commented 4 years ago
Owner

Since the MATCH used requires that it be a path containing /kbd, then it should work. Anyway, your last solution using kbd for the EXISTS test is very good.

Since the MATCH used requires that it be a path containing `/kbd`, then it should work. Anyway, your last solution using `kbd` for the `EXISTS` test is very good.
aneejit1 commented 4 years ago
Poster
Collaborator

Updates pushed, and I've amended the commit comments as well.

Updates pushed, and I've amended the commit comments as well.
ConfigureChecks.cmake Outdated
endif( )
if( X11_XKB_RULES_DIR )
string( REGEX MATCH ".$" LAST_CHAR "${X11_XKB_RULES_DIR}" )
SlavekB commented 4 years ago
Owner

Instead of detecting the last character separately, a condition containing NOT ... MATCHES could be used here:

    if( NOT "${X11_XKB_RULES_DIR}" MATCHES "/$" )
Instead of detecting the last character separately, a condition containing `NOT ... MATCHES` could be used here: ``` if( NOT "${X11_XKB_RULES_DIR}" MATCHES "/$" ) ```
SlavekB commented 4 years ago
Owner

The xkbd commits have now been squished into one.

Please, when squash commits, it is good to make a reasonable merge of the commit comment.

> The xkbd commits have now been squished into one. Please, when squash commits, it is good to make a reasonable merge of the commit comment.
SlavekB commented 4 years ago
Owner

@SlavekB could you test on older distros?

Successfully tested building on Wheezy.

> @SlavekB could you test on older distros? Successfully tested building on Wheezy.
SlavekB commented 4 years ago
Owner

There seems to be a need for further discussion.

When testing on Debian 9 (Stretch), xkbcomp is present and the path /usr/share/X11/xkb is found using pkg-config. In fact, on a clean build chroot, the specified path does not exist. However, it is used during build.

When testing on Debian 7 (Wheezy), the man test finds the path /usr/share/X11/xkb, but because the path does not exist on a clean build chroot, it is not used. Similarly, the xkbfile test detects the prefix /usr, this leads to the verification of /usr/share/X11, but because xkb/rules do not exist on a clean build chroot, it is not used.

Therefore, on Debian 7 (Wheezy), none of the identified paths are used. And here's the question, is this the correct behavior when the test using man very clearly points to /usr/share/X11?

There seems to be a need for further discussion. When testing on Debian 9 (Stretch), xkbcomp is present and the path /usr/share/X11/xkb is found using `pkg-config`. In fact, on a clean build chroot, the specified path does not exist. However, it is used during build. When testing on Debian 7 (Wheezy), the `man` test finds the path /usr/share/X11/xkb, but because the path does not exist on a clean build chroot, it is not used. Similarly, the xkbfile test detects the prefix /usr, this leads to the verification of /usr/share/X11, but because xkb/rules do not exist on a clean build chroot, it is not used. Therefore, on Debian 7 (Wheezy), none of the identified paths are used. And here's the question, is this the correct behavior when the test using `man` very clearly points to /usr/share/X11?
aneejit1 commented 4 years ago
Poster
Collaborator

The aim of the cmake changes was to have a much more targeted attempt to locate the Kbd rules file based on the information available and push that path to the top of the list in kxkb, if available (this as well as some additions to the list). If cmake doesn't find a path, or the path found doesn't exist, kxbd will continue checking the remaining items on the list. I've not changed kxbd's function, just given it some extra directories to test.

I've not managed to find the Debian 7 or 9 media yet as the website only seems to want to direct me to version 10. I have found the contents lists for versions 7 and 9, which would indicate that both have the xkeyboard-config package installed with the layout files under /usr/share/X11. The .pc file for xkeyboard-config is listed for both versions as being "/usr/share/pkgconfig/xkeyboard-config.pc", however, Version 9 has the xkbcomp file listed as "/usr/lib/x86_64-linux-gnu/pkgconfig/xkbcomp.pc" but is not listed for version 7 (the only references to xkbcomp I found are to the binary and two manpages).

Both xkeyboard-config.pc and xkbcomp.pc have variables which state where the directory is, therefore their existence is not checked. The manpage test does check existence mainly as a means of preventing something bad reaching the compiler if there is something wrong with the manpage such as a control character in the wrong place. The xkbfile.pc test has to search for the directory because the locations used as starting points are "{prefix}" and "{libdir}", not specific paths.

What you've said about Debian 9 suggests that "/usr/share/pkgconfig" directory is not listed in PKG_CONFIG_PATH as xkeyboard-config.pc should exist there and be picked up before xkbcomp is. If the same applies on Debian 7, then it will fall into the manpage and xkbfile tests which would require the path to exist. If your chroot environment does not have the rules directories installed, that would explain why paths from the second two tests are not used. kxkb does have "/usr/share/X11" in its list of paths, so it will still test and use it as part of its search process if the resultant binary is run from a location where the directory exists.

The aim of the cmake changes was to have a much more targeted attempt to locate the Kbd rules file based on the information available and push that path to the top of the list in kxkb, if available (this as well as some additions to the list). If cmake doesn't find a path, or the path found doesn't exist, kxbd will continue checking the remaining items on the list. I've not changed kxbd's function, just given it some extra directories to test. I've not managed to find the Debian 7 or 9 media yet as the website only seems to want to direct me to version 10. I have found the contents lists for versions 7 and 9, which would indicate that both have the xkeyboard-config package installed with the layout files under /usr/share/X11. The .pc file for xkeyboard-config is listed for both versions as being "/usr/share/pkgconfig/xkeyboard-config.pc", however, Version 9 has the xkbcomp file listed as "/usr/lib/x86_64-linux-gnu/pkgconfig/xkbcomp.pc" but is not listed for version 7 (the only references to xkbcomp I found are to the binary and two manpages). Both xkeyboard-config.pc and xkbcomp.pc have variables which state where the directory is, therefore their existence is not checked. The manpage test does check existence mainly as a means of preventing something bad reaching the compiler if there is something wrong with the manpage such as a control character in the wrong place. The xkbfile.pc test has to search for the directory because the locations used as starting points are "${prefix}" and "${libdir}", not specific paths. What you've said about Debian 9 suggests that "/usr/share/pkgconfig" directory is not listed in PKG_CONFIG_PATH as xkeyboard-config.pc should exist there and be picked up before xkbcomp is. If the same applies on Debian 7, then it will fall into the manpage and xkbfile tests which would require the path to exist. If your chroot environment does not have the rules directories installed, that would explain why paths from the second two tests are not used. kxkb does have "/usr/share/X11" in its list of paths, so it will still test and use it as part of its search process if the resultant binary is run from a location where the directory exists.
MicheleC commented 4 years ago
Owner

@aneejit1 thanks for fixing up the commit messages 👍

@aneejit1 thanks for fixing up the commit messages :+1:
SlavekB commented 4 years ago
Owner

The problem is not in the missing path for the pkg-config files, but in the fact that the specified pkg-config files did not exist on the older version of the system.

The problem with the missing /usr/share/X11/xkb directory is not that the path is incorrectly found, but that the files were not needed at build time and therefore the xkb-data package was not installed.

An effective solution should be to add the xkb-data package to build-deps in case x11-xkb-utils is older than 7.7.

The problem is not in the missing path for the pkg-config files, but in the fact that the specified pkg-config files did not exist on the older version of the system. The problem with the missing `/usr/share/X11/xkb` directory is not that the path is incorrectly found, but that the files were not needed at build time and therefore the xkb-data package was not installed. An effective solution should be to add the xkb-data package to build-deps in case x11-xkb-utils is older than 7.7.
MicheleC commented 4 years ago
Owner

An effective solution should be to add the xkb-data package to build-deps in case x11-xkb-utils is older than 7.7.

Yes, as discussed on jabber, that is probably something to look into, perhpas only for wheezy if possible.

> An effective solution should be to add the xkb-data package to build-deps in case x11-xkb-utils is older than 7.7. Yes, as discussed on jabber, that is probably something to look into, perhpas only for wheezy if possible.
SlavekB approved these changes 4 years ago
SlavekB left a comment
Owner

It looks good.

Note: For some tests, it is necessary to have xkb rules data present – in the case of DEB distributions, the xkb-data package that will need to be added to build-deps.

It looks good. Note: For some tests, it is necessary to have xkb rules data present – in the case of DEB distributions, the xkb-data package that will need to be added to build-deps.
SlavekB merged commit ae1e54e0d4 into master 4 years ago
SlavekB deleted branch feat/ask-pkgconfig-for-kb-rules 4 years ago
SlavekB added this to the R14.0.9 release milestone 4 years ago

Reviewers

SlavekB approved these changes 4 years ago
The pull request has been merged as ae1e54e0d4.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
SlavekB
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/tdebase#158
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'feat/ask-pkgconfig-for-kb-rules'

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

No
Yes