mattanger / ckb-next

RGB Driver for Linux and OS X
http://forum.corsair.com/v3/showthread.php?t=133929
GNU General Public License v2.0
656 stars 76 forks source link

CMake Conversion #32

Closed mattanger closed 6 years ago

mattanger commented 7 years ago

@light2yellow and I are both working on the conversion from qmake to cmake. We'll be using this issue to track progress and note issues related to building with cmake.

ghost commented 7 years ago

Should the ckb gui be really always hided in the statusbar and never appear in the Dock in macOS? I'm trying to write a plist config and for me personally it would be much better to just have it in the Dock without the need to press "Restore ckb" every time.

mattanger commented 7 years ago

I'm not sure I know what you're describing.

For the macOS, I don't really have an opinion either way. I say if you like it the way you've described, go for it!

ghost commented 7 years ago

Alright. Regarding VERSION file: @tatokis and I discussed this on irc and as a result we agreed to leave the file for the compatibility with qmake and just generate it with cmake, incrementing versions with every release right in CMakeLists.txt. And later fix ckb.pro to consume the newline in the file.

mattanger commented 7 years ago

Ok. Makes sense. It's a good to keep qmake support around for now.

tatokis commented 7 years ago

Wouldn't it be better to just keep the version file and modify it in such a way to have both cmake and read qmake from it?

It would also make more sense when bumping the version number, to see changes in the version file and not a cmake one.

ghost commented 7 years ago

Yes, probably the best idea.

mattanger commented 7 years ago

Fine with me. I'll just change the bash command to strip the trailing newline. Simple fix.

ghost commented 7 years ago

And don't forget to actually add it to the VERSION :)

mattanger commented 7 years ago

Wait this is for VERSION!? :wink:

And question for those who may have tried. Did anyone build with a trailing newline in version? It didn't seem actually cause a problem.

I've changed the .pro files to strip the newline, just in case though.

frickler24 commented 7 years ago

OK, now I have a version conflict with my PR #25: The newline is remarked :-)

mattanger commented 7 years ago

Ha. That pesky newline.

On Jan 19, 2017 1:32 PM, "Lutz" notifications@github.com wrote:

OK, now I have a version conflict with my PR #25 https://github.com/mattanger/ckb-next/pull/25: The newline is remarked :-)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mattanger/ckb-next/issues/32#issuecomment-273858547, or mute the thread https://github.com/notifications/unsubscribe-auth/AFM7GjTyxGchhFvFXbEnEn4R8gw59F4Uks5rT6w8gaJpZM4Lm_ND .

ghost commented 7 years ago

I almost managed to build it on macOS. However, ObjC file doesn't want to link:

...
[ 91%] Building C object src/ckb/CMakeFiles/ckb-next.dir/media_mac.m.o
/Users/alexey/Documents/ckb-next/src/ckb/media_mac.m:14:8: warning: 'AudioHardwareServiceGetPropertyData' is deprecated:
      first deprecated in macOS 10.11 [-Wdeprecated-declarations]
    if(AudioHardwareServiceGetPropertyData(kAudioObjectSystemObject, &propertyAddress, 0, NULL, &dataSize, &deviceID)
       ^
/System/Library/Frameworks/AudioToolbox.framework/Headers/AudioHardwareService.h:161:1: note:
      'AudioHardwareServiceGetPropertyData' has been explicitly marked deprecated here
AudioHardwareServiceGetPropertyData(    AudioObjectID                       inObjectID,
^
/Users/alexey/Documents/ckb-next/src/ckb/media_mac.m:24:9: warning: 'AudioHardwareServiceHasProperty' is deprecated:
      first deprecated in macOS 10.11 [-Wdeprecated-declarations]
    if(!AudioHardwareServiceHasProperty(deviceID, &propertyAddress2))
        ^
/System/Library/Frameworks/AudioToolbox.framework/Headers/AudioHardwareService.h:87:1: note:
      'AudioHardwareServiceHasProperty' has been explicitly marked deprecated here
AudioHardwareServiceHasProperty(    AudioObjectID                       inObjectID,
^
/Users/alexey/Documents/ckb-next/src/ckb/media_mac.m:28:8: warning: 'AudioHardwareServiceGetPropertyData' is deprecated:
      first deprecated in macOS 10.11 [-Wdeprecated-declarations]
    if(AudioHardwareServiceGetPropertyData(deviceID, &propertyAddress2, 0, NULL, &dataSize, &state)
       ^
/System/Library/Frameworks/AudioToolbox.framework/Headers/AudioHardwareService.h:161:1: note:
      'AudioHardwareServiceGetPropertyData' has been explicitly marked deprecated here
AudioHardwareServiceGetPropertyData(    AudioObjectID                       inObjectID,
^
3 warnings generated.
[ 93%] Building CXX object src/ckb/CMakeFiles/ckb-next.dir/ckb-next_automoc.cpp.o
[ 95%] Building CXX object src/ckb/CMakeFiles/ckb-next.dir/ckb-next_automoc.dir/qrc_image_M75OZXHIIGXO42.cpp.o
[ 96%] Building CXX object src/ckb/CMakeFiles/ckb-next.dir/ckb-next_automoc.dir/qrc_text_CHQ2YP46YACNZY.cpp.o
[ 98%] Building CXX object src/ckb/CMakeFiles/ckb-next.dir/ckb-next_automoc.dir/qrc_binary_ATZYOSS2HMTWAZ.cpp.o
[100%] Linking CXX executable ckb-next.app/Contents/MacOS/ckb-next
Undefined symbols for architecture x86_64:
  "_AudioHardwareServiceGetPropertyData", referenced from:
      _getMuteState in media_mac.m.o
  "_AudioHardwareServiceHasProperty", referenced from:
      _getMuteState in media_mac.m.o
  "_OBJC_CLASS_$_NSProcessInfo", referenced from:
      objc-class-ref in media_mac.m.o
  "___CFConstantStringClassReference", referenced from:
      CFString in media_mac.m.o
  "_objc_msgSend", referenced from:
      _disableAppNap in media_mac.m.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [src/ckb/ckb-next.app/Contents/MacOS/ckb-next] Error 1
make[1]: *** [src/ckb/CMakeFiles/ckb-next.dir/all] Error 2
make: *** [all] Error 2
make  41.70s user 4.90s system 89% cpu 52.009 total

UPD. I fixed the deprecated functions but undefined symbols remain:

Undefined symbols for architecture x86_64:
  "_AudioObjectGetPropertyData", referenced from:
      _getMuteState in media_mac.m.o
  "_AudioObjectHasProperty", referenced from:
      _getMuteState in media_mac.m.o
  "_OBJC_CLASS_$_NSProcessInfo", referenced from:
      objc-class-ref in media_mac.m.o
  "___CFConstantStringClassReference", referenced from:
      CFString in media_mac.m.o
  "_objc_msgSend", referenced from:
      _disableAppNap in media_mac.m.o

UPD2. It's finally building. The frameworks were not linking properly and CoreAudio was missing (I wonder how the hell qmake built this).

ghost commented 7 years ago

I have previously mentioned an issue with locating qt5 on macOS. One solution is to try to find it automatically, but this introduces Homebrew's qt5 package as a dependency and not from the official installer. But from what I carried out bitching about this on cmake's irc channel it should be a user's task to specify the right path to qt5 through -DCMAKE_PREFIX_PATH when executing cmake ... I disagreed that this is too tedious but even the official cmake does so if one wants to build from source. And this also removes a dependency on Homebrew's package. So I think it's better to leave a possibility for user to populate -DCMAKE_PREFIX_PATH with the right location while building and if it is empty, require Homebrew as a dependency. In any case this won't hurt a regular user because we can make a release and put drag-n-drop installer there.

fleischie commented 7 years ago

I have two comments for this issue:

Cmake on MAC

I got it to work with the following steps:

  1. Have XCode + CLI-tools installed.
  2. Install qt5 and cmake via brew: brew install qt5 cmake.
    • There was a warning of qt5, that it has not been linked, as it was a Cask-file only (or something, I don't understand the brew-terminology that well).
    • It told me to use LD and CMAKE flags, though.
  3. Add qt5's bin/ directory to PATH: export PATH=$PATH:<path_to_qt5>/bin
  4. Run cmake with the absolute path for the LD- & CPP-Flags: LDFLAGS=-L<absolute_path_to_qt5>/lib CPPFLAGS=-I<absolute_path_to_qt5>/include cmake ..
  5. ALTERNATIVELY: Link qt5 by force: brew link qt5 --force <- but this I did not do, because it's not the recommended way.

Cmake on Linux

I am not entirely sure how far the progress on the cmake-port is, but fyi: The animations couldn't be loaded, as they are looked for in the wrong directory, probably because they are not compiled.

Steps to reproduce:

  1. Clone the project.
  2. Checkout the cmake branch.
  3. Create a build/ directory in the project root and cd into it.
  4. cmake .. && make
  5. Start the daemon (su -c "./src/ckb-daemon/ckb-daemon") and open the GUI (./src/ckb/ckb).
  6. The animations where searched for in PROJECT_ROOT/build/src/ckb/ckb-animations.
ghost commented 7 years ago

@fleischie yep, this is still WIP, all issues (more like unimplemented features, not issues) are known and the prerequisites will be mentioned in the readme when it's ready. And you shouldn't pass the command line options directly in cmake :) P. S. you can look at my cmake-bustling branch, it has basic features for mac, almost nothing for linux (but easy to fill in). I'm currently going through exams and the last thing I fought with was mac bundling. There's a big project called openchemistry written by kitware employee, I'm trying to understand what's going on there and apply it onto this project. Qt5 deploying is not very obvious, at least for me.

mattanger commented 7 years ago

Definitely a wip. I've working on some Linux improvements that corrects the animations, but the quickinstall script is currently buggered so I haven't made an update.

On Feb 6, 2017 6:36 AM, "Oleksii Vilchanskyi" notifications@github.com wrote:

@fleischie https://github.com/fleischie yep, this is still WIP, all issues (more like unimplemented features, not issues) are known and the prerequisites will be mentioned in the readme when it's ready. And you shouldn't pass the command line options directly in cmake :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mattanger/ckb-next/issues/32#issuecomment-277657267, or mute the thread https://github.com/notifications/unsubscribe-auth/AFM7GtegKgS6ssuLsSPGUdBrxe8_NMY5ks5rZwXEgaJpZM4Lm_ND .

ghost commented 7 years ago

Just to track some progress: most of the job is done, pkg is building (although not tested for every case), macos support is full and needs just a few changes and a bit more testing, linux support is almost full and needs testing, quazip will be the tricky one because of the local includes that must be changed to global includes and your humble servant's lack of knowledge on how to pass paths, find libs, build and link the right libs the right way.

ghost commented 7 years ago

The main part is ready - both systems have installers and uninstallers, both tested and working. However on Linux #89 is required to work normally, so this is a prerequisite for CMake transition. I have no idea how this worked before, with qmake, as partially described in #82. I'm starting to work on Readme and Wiki. We will have new names for binaries, plists and their Labels.

frickler24 commented 7 years ago

@light2yellow I Just Tried to compile the cmake branch from your repo. first I noticed was a missing pulse-dev:

lutz@Mainfrix ~/Projekte/new-cmake-ckb-next/build $ cmake ..
-- Determined Source Version : 0.2.8-14-g0f5ef3c
-- Could NOT find PulseAudioSimple (missing:  PULSEAUDIOSIMPLE_LIBRARY PULSEAUDIOSIMPLE_INCLUDE_DIR) 
CMake Error at /usr/share/cmake-3.5/Modules/FindPackageHandleStandardArgs.cmake:148 (message):
  Could NOT find PulseAudio (missing: PULSEAUDIO_LIBRARY
  PULSEAUDIO_INCLUDE_DIR)
Call Stack (most recent call first):
  /usr/share/cmake-3.5/Modules/FindPackageHandleStandardArgs.cmake:388 (_FPHSA_FAILURE_MESSAGE)
  cmake/FindPulseAudio.cmake:57 (find_package_handle_standard_args)
  src/ckb-mviz/CMakeLists.txt:3 (find_package)

Fixed it with apt install libpulse-dev. Should we mention it in the install path of the README.md? Now its compiling...

frickler24 commented 7 years ago

Next error:

Scanning dependencies of target ckb-next_automoc
[ 43%] Automatic moc, uic and rcc for target ckb-next
Generating ui_animadddialog.h
Generating ui_animsettingdialog.h
Generating ui_extrasettingswidget.h
Generating ui_fwupgradedialog.h
Generating ui_gradientdialog.h
Generating ui_kbanimwidget.h
Generating ui_kbbindwidget.h
Generating ui_kblightwidget.h
Generating ui_kbprofiledialog.h
Generating ui_kbwidget.h
Generating ui_kblightwidget.h
Generating ui_kperfwidget.h
Generating ui_layoutdialog.h
Generating ui_mainwindow.h
Generating ui_modeselectdialog.h
Generating ui_mperfwidget.h
Generating ui_rebindwidget.h
Generating ui_settingswidget.h
RCC: Error in '/home/lutz/Projekte/new-cmake-ckb-next/src/ckb/text.qrc': Cannot find file 'ckb-next.plist'
AUTORCC: error: process for /home/lutz/Projekte/new-cmake-ckb-next/build/src/ckb/CMakeFiles/ckb-next.dir/qrc_text.cpp failed:
RCC: Error in '/home/lutz/Projekte/new-cmake-ckb-next/src/ckb/text.qrc': Cannot find file 'ckb-next.plist'

searching for it finds just the .in files:

lutz@Mainfrix ~/Projekte/new-cmake-ckb-next $ find . -name \*plist\*
./src/ckb-daemon/ckb-next-daemon.plist.in
./src/ckb/ckb-next.plist.in
ghost commented 7 years ago

@frickler24 thanks for testing this.

Should we mention it in the install path of the README.md?

Yes, thanks.

Next error:

Interesting, I don't experience it on Ubuntu 16.10, clean installation. I did cmake .. && make -j"$(nproc --all)" && sudo make install. What command did you use? Are you on Mint? Linux has nothing to do with plists.

ghost commented 7 years ago

Also, you can refer to https://github.com/light2yellow/ckb-next/blob/cmake-bustling/docs/LINUX_INSTALLATION.md Just pushed, but the essential info is there.

ghost commented 7 years ago

@frickler24 oh, never mind, I found the problem. CMake doesn't create plists on Linux because there's no need to, however, they are still mentioned in text.qrc. I will make it being generated as well.

ghost commented 7 years ago

@frickler24 should be fixed now. Later I'll make a proper generation for other qrcs too and gitignore them.

ghost commented 7 years ago

@frickler24 what was that problem? You must have done this without sudo, because this is not a CMake problem.

tatokis commented 7 years ago

From my understanding, compiled user-executable binaries go in /usr/local/bin, animations should go in /usr/local/lib/ckb-animations.

Package managers should install in /usr/bin and /usr/lib/ckb-animations respectibly. This is done to prevent collisions with package managers, and if a user wants to test a manually compiled version, they can do so easily without uninstalling the package from the repos.

Directories aren't allowed in any "bin" directory.

Now whether they are symlinks or not to /opt is another issue, but I honestly don't see why we should put it in /opt, considering that it is mostly used for completely 3rd party applications, often proprietary.

And yes, make install is ran as sudo to get the permissions required to install system-wide, it does not interfere with local build directory-related permissions.

frickler24 commented 7 years ago

@light2yellow OK, cmake .. && make with an additional sudo make install worked now. I added no parameter to the make call. The keyboard is not detected with this version, but this is not an error in the cmake config I think.

frickler24 commented 7 years ago

sorry, the posting without sudo was my fault.

frickler24 commented 7 years ago

the parallel make is funny: grafik. works also.

Now the K95RGB works again, I have trouble to restart the communication with both M65RGB and K95RGB active. I saved a lot of logging info, hope that I find the error.

BTW next weekend we should try to merge the delay fix #25, because with this enhancement the layout of the key macro definitions has changed and the UI a bit. without this I cannot senseful use the testing branch.

frickler24 commented 7 years ago

@light2yellow is there any reason why the installation of the old ckb-daemon is affected by the cmake installation? I stopped the ckb-daemon before installing ckb-next-daemon. After stopping ckb-next-daemon the installation of ckb-daemon was broken (only the ckb-daemon-restart .service was available, not the original ckb-daemon.service). Any idea?

ghost commented 7 years ago

@frickler24 this is by intention. ckb and ckb-next do not play well together. All references to ckb are removed (from the known paths) and all references of ckb-next are freezed and then replaced (again, on known paths).

ghost commented 7 years ago

@frickler24 99% of users will never manually uninstall ckb first, so there's that.

ghost commented 7 years ago

@frickler24 so you say building ckb-next with all cores breaks it? This is a bit mysterious again and more like make/setup problem.

frickler24 commented 7 years ago

No, parallel make works fine on mint. Parallel make is an application that makes sense and uses all my 4 cores + MT! Now my SSD seems to be too slow... As Seymour Cray said: A Supercomputer is a computer, which transforms computing-intensive problems into I/O-intensive problems. Something happens always :-)

The only one question was that for the old ckb version. I agree, your de-installation first is useful and will help to avoid many questions and problems. I just wondered, because I did not see the corresponding statement.

So for me the cmake runs fine after the apt install lilbpulse-dev.

This is tested on a mint 18.1 If you wish - I have a 17.3 (and I think a 17.2) also. But are there differences in make-structures?

ghost commented 7 years ago

Cool, thanks for providing a useful feedback. There's no need to test on earlier versions because I think things won't change at all. The only requirement is cmake 3.3. I'll also think about changing target file location as by @tatokis. However I like how it lives in opt too.

tatokis commented 7 years ago

Regarding libpulse, it should be an optional dependency (like with qmake), there are computers without pulseaudio, and libpulse is only used for a single plugin.

ghost commented 7 years ago

@tatokis I thought about it a bit more.

From my understanding, compiled user-executable binaries go in /usr/local/bin, animations should go in /usr/local/lib/ckb-animations.

Package managers should install in /usr/bin and /usr/lib/ckb-animations respectibly.

Okay. Next.

This is done to prevent collisions with package managers, and if a user wants to test a manually compiled version, they can do so easily without uninstalling the package from the repos.

This will lead to the collisions between daemons. I already look for two places now - to guarantee an older ckb is removed and a newer ckb-next is neutralized before replacing files. I don't want to make a third path. This is one argument. Another argument are collisions, the user cannot just install two versions simultaneously like you describe and hope for the best. And then report errors like "oh look I have two icons my kb types two letters at one press ples halp" would appear. This is another reason why the first thing I wrote here is that I don't know what would package maintainers use if they don't like current file layout. And the current file layout was established. You can see two packages in the AUR with this layout right now. And these are new, ckb-next. You can see two older packages in the AUR with a very similar structure. These are ckb. I am pretty sure @hevanaa uses the same structure as ckb, he didn't even change the package name (nevertheless you should've, @hevanaa , this is ckb-next, not ckb). And they are encouraged to stick to what was used before. Because it makes the life simpler. Or make sure they neutralize everything before their custom install and then clean up after themselves after uninstall.

Right now a CMake installation script tries its best to detect everything obsolete and inject new information the safest and the most stable way possible.

To sum up, I see no rationale in moving files out of /opt. Neither there's any profit. Useful binaries are softlinked into the PATH, so there's no difference at user level at all. This will only bring more ugly if's and elses and problems with users, who can't even execute sudo apt install X.

ghost commented 7 years ago

Today I sent this email to @mattanger:

Hi Matt!

Sorry for disturbing, but that's a quick question, shouldn't take much time.

I wanted to ask if I could be allowed to merge my fork branch "cmake-bustling" (https://github.com/light2yellow/ckb-next) with the project's "cmake" branch and overwrite it, which means merging would be basically replacing these branches, which practically means "accept everything from cmake-bustling, remove obsolete files from cmake"? This way I can resolve a merge conflict the way I see this and it will make somewhere into the upstream, so that everyone else will not have hard time looking for cmake version and still won't hurt anyone, because it's not merging into testing or master. I'm sending this as an email because there was no recent activity on github, but you'll definitely get and read this email even if github notifications are disabled. This email will be published on github as well.

Regards,

Oleksii Vilchanskyi

I'm also interested to hear about what other members think about this: @fleischie @tatokis @frickler24 - whether do you agree with it and whether it's a good idea, or the development should continue on the fork and upstreamed when a final version is ready, or any other combinations you can think of.

fleischie commented 7 years ago

I personally am wondering, why the cmake branch does not include all the newer changes already (i.e. why isn't it rebased onto the latest testing/master). Apart from that, I'd go with the newer cmake-bustling branch, as it seems to be further developed (including the animations etc.).

I do understand however, that changes always come with more maintenance.

Bottom line: I am for merging.

P.S.: How about we create a ckb-next organization, so we might omit the dilemma of losing the project's owner in the void again. (Not, that I am already thinking Matt Ranger is gone. :stuck_out_tongue_winking_eye: )

ghost commented 7 years ago

@fleischie cmake does not include them because nobody was touching it for a long time. So I instead propose replacing it with cmake-bustling and I'll continue to work on it right inside the project and not in an own fork.

I am for an organization. One reason is wider permissions (for example, it'd be neat if someone with the highest permissions (obviously Matt Ranger solely now) could've set up the tags for easier search and so on, just these small things) and the other is a possibility to create more repositories, for example we have a website now.

hevanaa commented 7 years ago

@light2yellow I've changed the Fedora rpm package name to ckb-next (still not published). Rpm takes care of obsoleting the "ckb" package (any ckb rpm package will be upgraded to ckb-next).

I'm wondering about versioning. I've used 0.2.7 from ckb (as per Fedora packaging guidelines 0.2.7-0.x.git means a pre-release or beta version of 0.2.7), but here in ckb-next the latest version (tag) is 0.2.4. I'm going to ignore that for now, it is bad practice to change to a lower version number (and I see that the arch package is also using 0.2.7. Anyway, it is built on master branch.

mattanger commented 7 years ago

@Everyone. Apologies for the absence! The real world has not been kind to my free time as of late! But I'm going to address this later today, and be back with more detail.

On Thu, Mar 2, 2017 at 7:19 AM, Johan Heikkilä notifications@github.com wrote:

@light2yellow https://github.com/light2yellow I've changed the Fedora rpm package name to ckb-next (still not published). Rpm takes care of obsoleting the "ckb" package (any ckb rpm package will be upgraded to ckb-next).

I'm wondering about versioning. I've used 0.2.7 from ckb (as per Fedora packaging guidelines 0.2.7-0.x.git means a pre-release or beta version of 0.2.7), but here in ckb-next the latest version (tag) is 0.2.4. I'm going to ignore that for now, it is bad practice to change to a lower version number (and I see that the arch package is also using 0.2.7. Anyway, it is built on master branch.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mattanger/ckb-next/issues/32#issuecomment-283638951, or mute the thread https://github.com/notifications/unsubscribe-auth/AFM7Gkfp5vEas4XgSafyoSX8kszA7DlYks5rhrPngaJpZM4Lm_ND .

ghost commented 7 years ago

@hevanaa thank you for changing the name :)

The lack of tags drives me up the wall too! We are sorry for this. However, I tagged v0.2.8 on my cmake-bustling branch and will inevitably tag it once again (with v0.2.9 or even v0.3.0) before a CMake-based release (and I guess we're all moving on it, right?), but that will be master, and the release will be made based on master, obviously. More like the release, because it will contain a lot of other work too, mostly new models and bugfixes. And there would finally be a .pkg for Mac as well. So, yeah, all I can ask right now is patience.

Actually I went to see how AUR packages make versioning and discovered that it's utterly broken! The latest version git describes with git describe --long --tags is v0.2.4-158-gb59d179 (for master). But as per this page it shouldn't really matter what version is in the package's web description, the version is evaluated at compile time (however, current one is higher then git describe could have ever produced). But that's for Arch though. So, yeah, maybe I've forgotten about this and that's why I tagged v0.2.8 specifically and not v0.2.5, I don't remember.

So there's that.

hevanaa commented 7 years ago

The renamed Fedora rpm is now published at https://copr.fedorainfracloud.org/coprs/johanh/ckb/ Running dnf update will replace existing ckb package with ckb-next for those using the repository (seems to be a handful or two based on the copr stats).

frickler24 commented 7 years ago

@light2yellow To your question: I'm for merging your cmake-bustling branch into the cmake branch. Our next release should base on cmake, going through some weeks in the testing branch. If we get a lot of hard to fix problems with it, our fallback is the qmake version for release. If cmake is not producing issues on the different platforms, qmake is history. The more interesting question is: What is exactly content of the next release and what is coming in the following release?

hevanaa commented 7 years ago

I noticed that you were discussing binary locations in linux. Traditionally in Fedora and Suse, daemons and other programs not started by the user are installed in /usr/libexec. See https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html. The animations are also currently placed in /usr/libexec/ckb-animations by the rpm, which I suspect is not correct. Are the animations called/referred to by the user program and not by the daemon? If that is the case and they are by definition libraries, then the correct place is /usr/lib/ckb-animations (and /usr/lib64/ckb-animations for 64-bit libraries), as said by @tatokis above.

tatokis commented 7 years ago

Yeah, they are just binaries executed by the ckb GUI on demand. They are useless standalone, and should not be called by the user. /usr/lib is used for any architecture-dependent binaries that should not be called by the user, so this makes sense.

@frickler24 I would like to keep qmake, for now at least, as an option for systems that can not run cmake for whatever reason, (for example if a module is missing from the repositories and it requires to compile cmake from source).

frickler24 commented 7 years ago

@tatokis This is a good point. Do we prefer the cmake version for an architecture and provide qmake if we get issues for the cmake version? My concern is that we need to maintain both the qmake version and the cmake version for a long time.

hevanaa commented 7 years ago

I have another question about paths. In src/ckb-daemon/devnode.c (and similarly in src/ckb/kbmanager.cpp) the device FIFO is defined like this

ifndef OS_MAC

const char *const devpath = "/dev/input/ckb";

else

const char *const devpath = "/var/run/ckb";

endif

For some reason, the Suse Linux rpm had patched this to also use /var/run instead of /dev/input. I've tried both ways and it seems to work fine on Fedora.

Do you have any idea why /var/run would be preferred? I'm thinking /dev/input would be the correct path.

fleischie commented 7 years ago

Be wary of the n in ifndef.

/var/run is only preferred on MacOS. 😉