Make WITH_USBIDS more visible, update usb.ids file #207
Merged
SlavekB
merged 1 commits from feat/usbisd_file
into master
3 years ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'feat/usbisd_file'
Deleting a branch is permanent. It CANNOT be undone. Continue?
this add more visibility (for packagers) to the former WITH_USBIDS option.
Here are a few things that need to be solved.
For consideration is whether to add support to use of the existing variable
WITH_USBIDS
if it is set?if( _PATH_USBIDS )
set( PATH_USBIDS "${_PATH_USBIDS}" )
message( STATUS "PATH_USBIDS: ${_PATH_USBIDS}" )
endif()
If is requested
WITH_SYSTEM_USBIDS
, but the file is not found andWITH_USBIDS
is not set, it should end up usingtde_message_fatal(...)
.{
#ifndef USBIDS_FILE
TQString db = "/usr/share/hwdata/usb.ids"; /* on Fedora */
TQString db( PATH_USBIDS );
If is not used
WITH_SYSTEM_USBIDS
thenPATH_USBIDS
value will not be set. I'm afraid it causes unexpected behavior.TQString db = USBIDS_FILE;
#endif
{
TQString db = locate("data", "kcmusb/usb.ids");
Using
TQString db = locate(...)
inside the{...}
block causes it to be a local variable within a given block. This is probably not intended purpose.0057a358eb
to175b724f0b
3 years agoThis looks reasonably. However, there are some little things that need to change.
##### look for the usb.ids file
find_file( _PATH_USBIDS name usb.ids
Here it could be added to support existing option
WITH_USBIDS
. For example:set( PATH_USBIDS "${_PATH_USBIDS}" )
message( STATUS "Looking for usb.ids file: ${_PATH_USBIDS}" )
else()
set( INCLUDED_USBIDS 1 )
I suggest a variable name:
USE_BUILTIN_USBIDS
.#cmakedefine WORDS_BIGENDIAN @WORDS_BIGENDIAN@
/* Define the path for the usb.ids file */
#cmakedefine PATH_USBIDS "@PATH_USBIDS@"
Instead of
#cmakedefine
, use#define
to define the constant always, even if it can have an empty value.#else
TQString db = USBIDS_FILE;
#endif
TQString db = "PATH_USBIDS";
Here you need to use
PATH_USBIDS
without quotation marks, as otherwise, instead of the value specified by the constant, the "PATH_USBIDS" string will be used. And such a string probably never will determine an existing file.175b724f0b
to6410b79a2d
3 years agoAutomake build was not tested, It does work anymore on my system.
WIP:add WITH_SYSTEM_USBIDS option.to Make WITH_USBIDS more visible, update usb.ids file 3 years agoI believe that testing and adjusting automake build is not needed. Everyone knows that the current is building using CMake. Here is more things that are not implemented in automake builds: merging translations into desktop files, selection and installation of TDM unit for systemd,…
Maybe it could be said that automake files are left here due to the fact that the generation of Apidox still depends on the common admin module and existence of Makefile.am files.
Great, it looks good. There are last little things that would be good to change.
set( USBIDS_FILE "${WITH_USBIDS}" )
message( STATUS "Using specified usb.ids file: ${USBIDS_FILE}" )
else()
find_file( PATH_USBIDS name usb.ids
I believe that instead of
name
(singular, lowercase letters), it should be as a keywordNAMES
(plural, capital letters). Or may be omitted.TQString db = USBIDS_FILE;
#endif
if (db.isNull() || !TQFile::exists(db))
Here I recommend using
isEmpty()
instead ofisNull()
. The empty string is valid => testing using theisNull()
can lead to an unexpected result.5720975484
to2797ae5a2b
3 years ago2797ae5a2b
to2a38c3e239
3 years ago2a38c3e239
to96e6e3105e
3 years ago96e6e3105e
to899c23fb9d
3 years agoIt looks good.
Please make rebase to current head.
899c23fb9d
to1389e29696
3 years ago1389e29696
into master 3 years agoReviewers
1389e29696
.