Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| PC_CFLAGS= | ||
| prefix=${prefix-/usr/local} | ||
| if test "${uname}" = "Haiku" || test "${uname}" = "haiku"; then | ||
| prefix=${prefix-/boot/system} |
There was a problem hiding this comment.
The Haiku equivalent of /usr/local is /boot/system/non-packaged. /boot/system only works for packages, because it is read-only.
There was a problem hiding this comment.
This is just an early patch, to get configure and cmake make identical binaries, after that I start working on package recipe. Supporting both non-packaged and packaged build makes things too complicated and suits better for downstream patch if needed.
| exec_prefix=${exec_prefix-'${prefix}'} | ||
| bindir=${bindir-'${exec_prefix}/bin'} | ||
| libdir=${libdir-'${exec_prefix}/lib'} | ||
| if test "${exec_prefix}" = "/boot/system" || (test "${prefix}" = "/boot/system" && test "${exec_prefix}" = '${prefix}'); then |
There was a problem hiding this comment.
This check doesn't work like this. Packages use a prefix like /packages/$packageName/.self (which at runtime is a symlink to where the package is installed, usually /boot/system), or it is a non-packaged path (see above).
The same applies also everywhere else where you check the prefix.
There was a problem hiding this comment.
Can't just evaluate exec_prefix because it isn't always variable reference, hence single quotes, it can be a literal path. When packaging, prefix gets overwritten.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1798 +/- ##
===========================================
+ Coverage 33.09% 33.16% +0.07%
===========================================
Files 66 66
Lines 5490 5490
Branches 1225 1225
===========================================
+ Hits 1817 1821 +4
+ Misses 3408 3407 -1
+ Partials 265 262 -3 ☔ View full report in Codecov by Sentry. |
| # Determine installation directory for man files | ||
| if (DEFINED MAN_INSTALL_DIR) | ||
| set(MAN_INSTALL_DIR "${MAN_INSTALL_DIR}" CACHE PATH "Installation directory for manuals (Deprecated)" FORCE) | ||
| set(CMAKE_INSTALL_MANDIR "${MAN_INSTALL_DIR}") | ||
| elseif (DEFINED INSTALL_MAN_DIR) | ||
| set(CMAKE_INSTALL_MANDIR "${INSTALL_MAN_DIR}") | ||
| elseif (CMAKE_INSTALL_PREFIX STREQUAL "/boot/system") | ||
| # Haiku system manuals | ||
| set(CMAKE_INSTALL_MANDIR "documentation/man") |
There was a problem hiding this comment.
Why do we need to add MAN_INSTALL_DIR and INSTALL_MAN_DIR if they were not used before, instead of using the standard CMAKE_INSTALL_MANDIR variable?
MAN_INSTALL_DIR has been removed as unused: #885
There was a problem hiding this comment.
This is just to document all the directories, so I can run pkgcheck and see it pass... There is still a lot of unused code in both configure and CMakeLists.txt for backwards compatibility.
Haiku is open-source operating system forked from BeOS. It doesn't use same file layout as Linux or *BSD.
We have to move uname check earlier, so defaults can be selected depending on operating system.