https://bugzilla.redhat.com/show_bug.cgi?id=2379721 --- Comment #18 from Claire Robsahm <inquiries@xxxxxxxxxxx> --- >(In reply to Cristian Le from comment #17) > > 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. Fixed. > --- > > 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. Yeah, I am aware. This was just the way I could get it to work. I could probably with autosetup get it in a better place, using your fetchcontent flags, but this is working just fine, and I think I prefer it. Will change if necessary. > --- > > When using %autorelease, please also use %autochangelog and vice-versa. > There are weird breakages if only one of them is used. Fixed. > --- > > 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. I remember running into issues when I only changed the flag, but I might be misremembering. Fixed, will test and see if everything is kosher. > --- > > 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. > > --- The install wizard is really a helper to help find the data files for the game you want to play. By default, it helps locate the data files of the "well-known" roleplaying game (Morrowind), but can also locate the datafiles of other openMW compatible games. The name installation makes it somewhat confusing, but it's an essential tool for ease of use. > 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? > > --- Upstream has a preferred commit to use from recastnavigation due to regressions. Otherwise, only godot also uses 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` Added information on extern. > --- > > Is `openmw-navmeshtool` not a `-tool` binary? > > --- On first blush, yes, except the other tools are run from commandline. `openmw-navmeshtool` is part of `openmw-launcher`; the user can run it from the navigation mesh tab to pregenerate navmeshes, improving load times. Because it's part of the launcher, I included it in the main package. > 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. So, I ended up talking to upstream and they found that the test always fails when run alone regardless of compiler. The issue we were having simply exposed this issue. My merge request has been approved to fix this, see: https://gitlab.com/OpenMW/openmw/-/merge_requests/4807. For now, I'm patching the MR in, since this is building the 0.49.0 release, not the latest commit. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component 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%23c18 -- _______________________________________________ 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
