https://bugzilla.redhat.com/show_bug.cgi?id=2379721 Cristian Le <fedora@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@xxxxxxxxxxxxxxxxx |fedora@xxxxxxxxx Status|NEW |ASSIGNED CC| |fedora@xxxxxxxxx --- Comment #17 from Cristian Le <fedora@xxxxxxxxx> --- > I'm too exhausted to figure out why at the moment, but here's the updated links おつかれさま。The big hurdle for this package seems to be done, now there are just a few details. I'll also address some questions from the matrix chat. I will write a shorter package review once I can get fedora-review to work again and help me with the license review. --- Some build dependencies are unused like `dos2unix`, `gnuplot`, `doxygen`, etc. If you check the built binaries/rpm, you could maybe deduce the minimum required from what packages are being linked. --- You can use %forgesetup as a simplification for %setup. You did mention that you didn't know what the difference between -p0 and -p1 is. Basically if you look at a patch, it can look like ``` --- a/CMakeLists.txt 2025-07-08 17:22:57.205176869 -0700 +++ b/CMakeLists.txt 2025-07-08 17:24:13.512715692 -0700 ``` -pN flag says to strip the first N paths (-p1 here would be a/ and b/) before applying the patch (which is done effectively in the git repo of SOURCE0, you can see the cd command in the %prep log of build.log). Generally it is best to use -p1 format because that's what github/gitlab will give you when you ask for their patch files. When writing patches you have to keep in mind: - to use consistent `-pN` across the patches, and account for the location of the patches after everything is extracted - the order of patches matter - if you want to use %autosetup with more than a Source0, you need to add -a flag for the other sources as well [1] Regarding extracting all of the other SourceN, this is done with -a flag, but keep in mind that the extracted folder of these will be less predictable. In your case you can use wildcard expansion, create symlinks in the prep phase, and so on. E.g. ``` -DFETCHCONTENT_SOURCE_DIR_BULLET:PATH=$(echo ./bullet-*) ``` On a small side-note `%{_builddir}` is not the CMake build-dir, it's the path where the RPM is being built in (I think one directory higher than the cwd in those commands). Just a random note in case you were not aware of it. --- When using %autorelease, please also use %autochangelog and vice-versa. There are weird breakages if only one of them is used. --- The CMakeLists.patch is unnecessary. Instead use ``` -DGLOBAL_DATA_PATH:PATH=%{_datadir} ``` Anything that has `option` or `set(CACHE)` you can change via `-D` variables. --- There are a lot of options that are worth going through and see if they need to be altered. One that catches my eye is `BUILD_WIZARD`. How is the "Installation Wizard" supposed to work, and particularly can it interfere with the installed files that are packaged? Most of the flags you would be having the best knowledge to judge. --- Regarding the previous comments about (de)bundling, these are best addressed with the other peers that use similar dependencies. I am not involved in this part of the ecosystem so I can't advise on future steps for this. But all of the reasoning you've documented look good, except for RecastNavigation. Do you want to put a TODO note about wanting to package it and what other package might use it? --- `extern/` path shows that there a few other dependencies that should be marked as `bundled`. Although they might not be used. If they are not used, you can avoid the `bundled` and just remove the folder. If you want to be extra careful, check the license of those files and make sure it is in Allowed License even if they are not used and do not appear in the `License` --- Is `openmw-navmeshtool` not a `-tool` binary? --- About the usage of `clang` for toolchain, if you want a smaller hammer, you could instead filter out the test in question, maybe ``` ./%{_vpath_builddir}/openmw-tests --gtest_filter="-test1:-test2" ``` The gtest executable has a good --help if you need check how to interact with it. Using clang toolchain is also fine, just keep in mind that there can be some weirdness when linking libraries built with other toolchains. --- License review: WIP --- [1]: https://rpm.org/docs/4.20.x/manual/spec.html#build-scriptlets -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2379721 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202379721%23c17 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
