Spec URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2.spec SRPM URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2-2.5.4-1.fc44.src.rpm Description: Chatterino 2 is a chat client for Twitch.tv. The Chatterino 2 wiki can be found https://wiki.chatterino.com/. Fedora Account System Username: solomoncyj
Note that this is a re-review of a retired package
Copr build: https://copr.fedorainfracloud.org/coprs/build/9599748 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2398039-chatterino2/fedora-rawhide-x86_64/09599748-chatterino2/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
*** Bug 2389308 has been marked as a duplicate of this bug. ***
In order to make the review a bit quicker, can you give me a diff with the last version before retirement? Just once is enough, not for every re-upload. Re copr issue, could you raise an issue with the packager to use cmake build?
Also, @rasmus.karlsson as you are involved with upstream, I will have some comments around CMake, and would greatly appreciate your help. I can help review of course.
(In reply to Cristian Le from comment #5) > Also, @rasmus.karlsson as you are involved with upstream, I will > have some comments around CMake, and would greatly appreciate your help. I > can help review of course. Of course, happy to help answer any questions or solve things upstreams where it makes sense.
@rasmus.karlsson Some preliminary requests, non-blockers for this review, but if they can be added quick, it would make reviewing and maintenance much easier: - Please gate all `add_sanitizers` and co. so that it can be safely removed. For the most part these do not have much benefit in the downstream packaging. It of course could be useful for catching issues in third-party libraries, but would you be willing to track those with us? - Drop as many `Find<Module>.cmake` as possible. If you have concerns with some of them that cannot be dropped, please file them as an issue to be tracked (can ping me on those @LecrisUT) - Linked to the above, please use `find_package(CONFIG)` + `FetchContent` compatibility, particularly for bundled components. The upstream might not be compatible with `FetchContent` in which case you can either: - Point to your forks, particularly when upstream is dead - Include a patch for the `FetchContent` part linked to an upstream PR (`PATCH_COMMAND`) if the upstream is active (Windows might be fussy about this though)
(In reply to Cristian Le from comment #7) > @rasmus.karlsson Some preliminary requests, non-blockers for > this review, but if they can be added quick, it would make reviewing and > maintenance much easier: > - Please gate all `add_sanitizers` and co. so that it can be safely removed. > For the most part these do not have much benefit in the downstream > packaging. It of course could be useful for catching issues in third-party > libraries, but would you be willing to track those with us? I've made an issue upstream making all sanitizer-related functionality opt-in https://github.com/Chatterino/chatterino2/issues/6492 > - Drop as many `Find<Module>.cmake` as possible. If you have concerns with > some of them that cannot be dropped, please file them as an issue to be > tracked (can ping me on those @LecrisUT) > - Linked to the above, please use `find_package(CONFIG)` + `FetchContent` > compatibility, particularly for bundled components. The upstream might not > be compatible with `FetchContent` in which case you can either: > - Point to your forks, particularly when upstream is dead > - Include a patch for the `FetchContent` part linked to an upstream PR > (`PATCH_COMMAND`) if the upstream is active (Windows might be fussy about > this though) Happy to explore this! I'll see about using `FetchContent` for our submodule dependencies. If you have an example of a CMake project using FetchContent for its dependencies that you think is particularly easy to review/maintain, I'd love to use it as a reference.
(In reply to Rasmus Karlsson from comment #8) > Happy to explore this! I'll see about using `FetchContent` for our submodule > dependencies. > If you have an example of a CMake project using FetchContent for its > dependencies that you think is particularly easy to review/maintain, I'd > love to use it as a reference. I have various ones scattered all around with various degree of refinement. Ideally I want to gather it back in my template [1], but as usual time and priority get in the way. At the top of my head, I think there are a few you can check: - https://github.com/spglib/spglib/blob/b2d6545e3f7b6479d6ba326c5415ce87f69260af/test/CMakeLists.txt#L87-L94 (Requires CMake 3.24 minimum) - https://github.com/adobe-type-tools/afdko/pull/1784 (Has backports for pre-CMake 3.24) - I had one more for how to support local submodules but can't find it, it is similar to the one above though. [1]: https://github.com/LecrisUT/CMake-Template/blob/ad3fe1f1871b54f05d371a97259aa74e40a4be60/CMakeLists.txt#L48-L59
ok I need to package https://github.com/ThePhD/sol2 first, so it will take a bit
[fedora-review-service-build]
Copr build: https://copr.fedorainfracloud.org/coprs/build/9603517 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2398039-chatterino2/fedora-rawhide-x86_64/09603517-chatterino2/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
@solomoncyj please use the "Spec URL" form of triggering, otherwise you are going to confuse the heck out of the fedora-review bot and it will not post diffs of the spec file. Or just do `fbrnch update-review`
diff tl;dr: added sol2 and miniaudio dependency, remove clang toolchain and go back to cpp, make -> ninja
Spec URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2.spec SRPM URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2-2.5.4-1.fc44.src.rpm
Copr build: https://copr.fedorainfracloud.org/coprs/build/9603531 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2398039-chatterino2/fedora-rawhide-x86_64/09603531-chatterino2/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Copr build: https://copr.fedorainfracloud.org/coprs/build/9603597 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2398039-chatterino2/fedora-rawhide-x86_64/09603597-chatterino2/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Copr build: https://copr.fedorainfracloud.org/coprs/build/9603630 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2398039-chatterino2/fedora-rawhide-x86_64/09603630-chatterino2/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Spec URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2.spec SRPM URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2-2.5.4-1.fc42.src.rpm
Created attachment 2107681 [details] The .spec file difference from Copr build 9603630 to 9603676
Copr build: https://copr.fedorainfracloud.org/coprs/build/9603676 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2398039-chatterino2/fedora-rawhide-x86_64/09603676-chatterino2/fedora-review/review.txt Found issues: - No gcc, gcc-c++ or clang found in BuildRequires Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/chatterino2 Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
@fedora anything else we need to change?
Ah ok, let me go through the list of improvements: - Use forgeurl consistently all-around (i.e. for the submodules as well). You can check openmw [1] for a reference. In this case you will still need to mv or symlink to the submodule paths as part of `%prep` - Try to use `CHATTERINO_SKIP_GIT_GEN` instead of specifying the git commits. @rasmus.karlsson I expect that would not create any user-facing issue? Otherwise let's discuss some alternatives. - The `-DBUILD_SHARED_LIBS=OFF` should be relaxed at some point, please make an issue to track what will be needed (probably a sub-task of the FetchContent refactor) - There is a PR for being able to drop `sanitizers-cmake`, please track that - The `uuid` macro seems excessive being used in a single location - Put a note on the `Requires: hicolor-icon-theme` that it is because directory ownership - Are the `qt6-*` Requires needed, and if so, by what? - Note for @rasmus.karlsson, `-DUSE_SYSTEM_QTKEYCHAIN=ON` should be the default, or track why the other way is preferred. It does smell like security sensitive. - BUILDING_ON_LINUX.md doc is meaningless for the user - The wildcards are a bit excessive, and I would recommend not to use it like that. E.g. if a new desktop file is created, you should check that any associated files are also present. - The docs folder seem to have primarily plugin developer documentation and some upstream documentation files? Better to filter the contents of that a bit. Be especially careful what suffixes you package because they might inject additional dependencies - Keep a note tracking the plugin design issue that we need to discuss Still TODO: - License check - Review the bundled sources (license check and purpose) - Review the default CMake options [1]: https://src.fedoraproject.org/rpms/openmw/blob/rawhide/f/openmw.spec
> - Try to use `CHATTERINO_SKIP_GIT_GEN` instead of specifying the git commits. @rasmus.karlsson I expect that would not create any user-facing issue? Otherwise let's discuss some alternatives. The About page will provide a little less information (i.e. no link to the upstream commit hash it was built from). We use it for error reporting when users run non-stable builds, so it won't impact users or their reports since we're packaging the stable version. The way @solomoncyj has supplied the upstream commit hash is the correct way, but I see it's unwieldy. Upstream could instead allow the relevant information to be specified as CMake options. The environment variable reading comes from some old Jenkins CI build system where the git repository wasn't available to the builder but these environment variables were. > - Note for @rasmus.karlsson, `-DUSE_SYSTEM_QTKEYCHAIN=ON` should be the default, or track why the other way is preferred. It does smell like security sensitive. That's fair - I'll track this. Many of the dependency issues come from us having a majority-Windows user base, where there's no "system" packages. It's better with conan / vcpkg nowadays, but we have some cleanup left. Much of this is being cleaned up as I go through the FetchContent refactor.
Thanks for the collaboration on these issues. (In reply to Rasmus Karlsson from comment #26) > The About page will provide a little less information (i.e. no link to the > upstream commit hash it was built from). We use it for error reporting when > users run non-stable builds, so it won't impact users or their reports since > we're packaging the stable version. Could you provide a free-form string that we can put `fedora` or equivalent inside it?
(In reply to Cristian Le from comment #27) > Could you provide a free-form string that we can put `fedora` or equivalent > inside it? The text on the About page includes a string generated by Qt [1][2] containing the user's system info. Example from Chatterino 2.5.2 running on my Fedora system: Chatterino 2.5.2 (commit 3aead09) built with Qt 6.8.1 (running on 6.9.2) Running on Fedora Linux Asahi Remix 42 (Forty Two [Adams]), kernel: 6.16.8-400.asahi.fc42.aarch64+16k Would a build-generated string still be interesting? [1]: https://doc.qt.io/qt-6/qsysinfo.html#prettyProductName [2]: https://doc.qt.io/qt-6/qsysinfo.html#kernelVersion
> Would a build-generated string still be interesting? That's on you to decide :P. But yes, my thought was to add the NVR of the package (name-version-release). If you wish to expose the bug report page as well, we can also redirect it to bugzilla if you would like and we can triage the upstream/downstream relevant bugs or do it the other way around.
(In reply to Cristian Le from comment #29) > > Would a build-generated string still be interesting? > > That's on you to decide :P. But yes, my thought was to add the NVR of the > package (name-version-release). If you wish to expose the bug report page as > well, we can also redirect it to bugzilla if you would like and we can > triage the upstream/downstream relevant bugs or do it the other way around. Ah yeah, I see how that would be helpful. I'll look into making that available as a CMake option. Thank you!
Spec URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2.spec SRPM URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2-2.5.4^20251110.f355928-1.fc44.src.rpm - Uncommitted changes
Created attachment 2114261 [details] The .spec file difference from Copr build 9603676 to 9793611
Copr build: https://copr.fedorainfracloud.org/coprs/build/9793611 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2398039-chatterino2/fedora-rawhide-x86_64/09793611-chatterino2/fedora-review/review.txt Found issues: - No gcc, gcc-c++ or clang found in BuildRequires Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/chatterino2 Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
@solomoncyj did you run fedora-review when you copied the licensecheck info in those headers? If not, please do. Here is the breakdown that I got from it ``` chatterino2: MIT AND BSD-3-Clause AND UNKNOWN!! AND {{ statically-linked dependencies (not yet reviewed) }} - Main is MIT - cmake/CodeCoverage.cmake: BSD-3-Clause (CMake helper only, does not propagate to binary, SourceLicense only) - lib/lrucache: BSD-3-Clause - src/providers/twitch/ChatterinoWebSocketppLogger.hpp: BSD-3-Clause (not in resources/licenses afaict) - semver/include/semver/semver.hpp: MIT - lib/twitch-eventsub-ws: UNKNOWN!! (@rasmus.karlsson please confirm) - resources/*: MIT (resources/contributors.txt has the details, unclear if they still own the copyright or surrendered it, assume former unless specified otherwise) libcommuni: BSD-3-Clause - Main is BSD-3-Clause settings: MIT - Main is MIT - cmake/conan_provider.cmake: MIT (CMake helper only, does not propagate to binary, SourceLicense only) signals: MIT - Main is MIT - cmake/conan_provider.cmake: MIT (CMake helper only, does not propagate to binary, SourceLicense only) serialize: MIT - Main is MIT magic_enum: MIT - Main is MIT - cmake/GenPkgConfig: UNLICENSE (CMake helper only, does not propagate to binary, SourceLicense only) certify: BSL-1.0 - Main is BSL-1.0 expected-lite: BSL-1.0 - Main is BSL-1.0 ``` Unless it is specified, please consider the licenses as joined by `AND` instead of `OR`, the latter has too much of a nuanced meaning to it. In some cases I added duplicate license because they are different copyright holders, but I didn't check all of them (man I wish they would just use spdx format and we could automate the copyright holders) @rasmus.karlsson, your licenses in `resources/licenses` are out-of-date. Could you do an independent review on your side as well and compare with our review to see if we all agree and didn't miss anything? Note that you only need to add the licenses if you link statically, so it would be nice if there was some logic to filter based on how this is built.
In our precaution (and lack of knowledge), we've included & attributed the license of every dependency we directly include, regardless of if it's a header only library, statically linked, or shared linked. Licenses for certify and pajlada/serialize are being fixed in https://github.com/Chatterino/chatterino2/pull/6575 src/providers/twitch/ChatterinoWebSocketppLogger.hpp uses the same license as websocketpp, whose license we have included under resources/licenses/websocketpp.txt, and is already included in our About page. lib/twitch-eventsub-ws is not an external library, but another module of Chatterino (it lives in the same repo, no submoduling), so licensed under the same Chatterino license. We could include the same license in that directory if that would help clarify things. resources/avatars contains the github avatar of contributors resources/{buttons,scrolling,settings,sounds,split,switcher,themes,automod} contains buttons/icons used in the app and are made by https://github.com/microsoft/fluentui-system-icons (we've included their licenses under resources/licenses and in our About page), by Chatterino contributors, or Twitch-owned badges used (as far as I know) in accordance to Twitch's developer agreement. resources/examples contains some videos used as tutorials for the applications, made by us. resources/licenses contains licenses of any asset/library we use resources/qss contains stylesheet for Chatterino, made by us resources/raw contain images & icons that are not included in Chatterino, but that we use to generate images that we use resources/contributors.txt contain a list of users who have helped contribute to Chatterino. We don't ask contributors to sign any CLA, so each contributor still owns their contributions, but the contributors.txt list is not complete. Our git commit history would be the closest we can get to a full "who technically owns what code" list. resources/emoji.json is a list of emojis / emoji sets & their capabilities. We attribute this and the emoji sets we include inside our About page. The rest of the included licenses are included correctly as far as I could tell, with the only thing I could see being wrong is technically the copyright year has been updated in some upstreams, which we haven't updated in a while. > Note that you only need to add the licenses if you link statically, so it would be nice if there was some logic to filter based on how this is built. Do header-only libraries fall under the same category as statically linked libraries for the purpose of licenses you need to include? We could make this a little bit easier by adding a comment to the top of each license file in resources/licenses/ with information about how we use it, i.e. whether the license covers assets, it's a header only library, statically linked, or shared linked.
> src/providers/twitch/ChatterinoWebSocketppLogger.hpp uses the same license as websocketpp, whose license we have included under resources/licenses/websocketpp.txt, and is already included in our About page. License is one thing, the copyright is another. The copyright holders are different in there. Technically not a big issue since the copyright is still preserved in the files, but depends on how you use those resources/license. > lib/twitch-eventsub-ws is not an external library, but another module of Chatterino (it lives in the same repo, no submoduling), so licensed under the same Chatterino license. We could include the same license in that directory if that would help clarify things. It's ok, just informal clarification is also ok. The eyebrow got raised when it was referencing some other page in the readme so just needed some clarification. If you could add spdx and copyright to the files than our licensecheck software will give you a cookie :) > resources/emoji.json is a list of emojis / emoji sets & their capabilities. We attribute this and the emoji sets we include inside our About page. Emojis and Unicode license is "quite the topic". Maybe you have already researched that topic? > The rest of the included licenses are included correctly as far as I could tell, with the only thing I could see being wrong is technically the copyright year has been updated in some upstreams, which we haven't updated in a while. The one that I noticed was - licenses/crashpad.txt is Apache-2.0 - Upstream is MIT https://github.com/Chatterino/crash-handler/blob/c8cf62b64906f794ac84cb0c22ceb401f4ea1e8d/LICENSE > Do header-only libraries fall under the same category as statically linked libraries for the purpose of licenses you need to include? Header-only and static libraries, yes, you own those symbols so you also inherit those licenses. The less known case is C++ templates, that also technically applies, but it is hard to detect and enforce. I don't know of requests on us to enforce that, but for header-only and static, we should be enforcing it and tracking CVEs accordingly. > We could make this a little bit easier by adding a comment to the top of each license file in resources/licenses/ with information about how we use it, i.e. whether the license covers assets, it's a header only library, statically linked, or shared linked. There is a standard for this. In my previous job it used `qt_attribution.json` [1], but I believe that is derived from some other standard on SBOM and SPDX. Kitware/CMake also want to invest in a solution on that last I known. Re: statically vs shared linked, you can only determine from CMake, so you need some simple `configure_file` in there, in which case, you could also conditionally add the `About` for features that are not being built. But not a big priority, so up to your discretion. [1]: https://wiki.qt.io/Qt_attribution.json
(In reply to Cristian Le from comment #36) > If you could add spdx and copyright to the files than our > licensecheck software will give you a cookie :) I'll add this to my todo-list, I'm not opposed to adding this to the top of our source files. I imagine similar to how Qt does it would work? https://github.com/qt/qtbase/blob/5fd393cb69be0c1def42023d59a46537484587cb/src/corelib/io/qbuffer.h > Emojis and Unicode license is "quite the topic". Maybe you have already > researched that topic? I have not. I am working under the assumption that emojis (i.e. the bytes to emoji translation) was free to use without any crediting / attribution, while emoji images (e.g. from the google emoji set we allow users to select) require attribution. > The one that I noticed was > - licenses/crashpad.txt is Apache-2.0 > - Upstream is MIT > https://github.com/Chatterino/crash-handler/blob/ > c8cf62b64906f794ac84cb0c22ceb401f4ea1e8d/LICENSE Crashpad refers to chromium's crashpad https://github.com/chromium/crashpad whereas crash-handler is our project that wraps around crashpad to ensure it works as expected on various platforms Thank you for your insight!
so confirming the license it will be : MIT AND BSD-3-Clause AND BSL-1.0 # BSL-1.0 # ----------------------------------------------------------------------- # resources/licenses/boost_boost.txt # lib/expected-lite/ # # BSD-3-clause # --------------------------------------- # lib/libcommuni/ # # MIT License # ------------- # lib/serialize/ # lib/signals/ # lib/magic_enum/ # resources/ # #
> I have not. I am working under the assumption that emojis (i.e. the bytes to emoji translation) was free to use without any crediting / attribution, while emoji images (e.g. from the google emoji set we allow users to select) require attribution. For reference, where I had that experience is when packagcing unicode-properties [1], and they also do not use the emoji art itself there [2]. Can't say I have more insight on it, other than, this is an unexpected license edge-case for me as well. > so confirming the license it will be : MIT AND BSD-3-Clause AND BSL-1.0 Yep, sounds about right, but be more explicit in the license breakdown, e.g. `lib/lrucache` should also be listed since it can be de-bundled. I think this is almost good to go, just a few more tasks: - Correct the license notes - Replace `g++` with `gcc-c++` - `doxygen` is not a requirement, let's avoid it - Please avoid wildcards if they are not needed, e.g. `%{_metainfodir}/*.xml` And just a modernization tip on how to make it more compact with %forgemeta `` %global forgeurl0 https://github.com/Chatterino/chatterino2 %global version0 2.5.4 %global date 20251110 %global commit0 f3559280786dbabc8cebe1951a525580d11cb2a5 %forgemeta -a Name: span Version: %forgeversion Release: %autorelease ``` The order of `%forgemeta` and `%autorelease` here makes it avoid any duplicate information, and `%forgeversion` takes care of all the version formatting for you. You can use `-p` to mark it as a pre-release, but I don't believe you want it in this case. Please also make a comment on why you take a snapshot. Most likely you can drop it or pick it back up as you need to with minimal diff. You do not need to use the full sha for the `commit<n>`, you can just use the short one and avoid an additional variable. `` %prep # Unpack the main source archive and the secondary sources # into separate directories in %%{_builddir}. # Then, switch into the unpacked Source0 directory. %setup -q %{forgesetupargs0} -b2 -b3 -b4 -b5 -b9 -b11 -b12 %autopatch -p1 mv %{_builddir}/%{topdir2} lib/libcommuni mv %{_builddir}/%{topdir3} lib/settings mv %{_builddir}/%{topdir4} lib/signals mv %{_builddir}/%{topdir5} lib/serialize mv %{_builddir}/%{topdir9} lib/magic_enum mv %{_builddir}/%{topdir11} lib/expected-lite mv %{_builddir}/%{topdir12} lib/certify ``` This one I took from openmw, haven't personally used it myself, but I like how it already does the extraction. Might not have the setup quite right, but play around with it as desired. And if you have a script that you think you could use for updating the submodules, please include it here for review. If you don't have any, that is ok, I will share mine with the devel channel once I get to it. [1]: https://github.com/unicode-rs/unicode-properties/ [2]: https://github.com/unicode-rs/unicode-properties/blob/558c2bf2a7218210218b861f416ec0bda9a7747b/scripts/unicode.py#L13-L17
Spec URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2.spec SRPM URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2-2.5.4^20251110gitf355928-1.fc44.src.rpm final fixes
Created attachment 2114660 [details] The .spec file difference from Copr build 9793611 to 9802379
Copr build: https://copr.fedorainfracloud.org/coprs/build/9802379 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2398039-chatterino2/fedora-rawhide-x86_64/09802379-chatterino2/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Seems like I mislead you with the improvements suggestion. I did a bit of local debugging and here is what I have: - Looks like you still need the full commit hash because internally github still uses the full commit hash in the folder name. So I guess you can revert this part. You could also use `%forgeversion -z <n>` and let it fill in all the stuff there - The mv command there would make a subfolder under it. Use `mv -T` to overwrite the folders - `%forgesetup -a` is broken due to the order in which they are processed, but surprisingly, you can call `%forge(auto)setup -z 0` after the `%forgesetup -a` and it works. The manual `%setup` before is also fine, it just that it requires keeping track of the forgemeta urls, but you can sol do it like this ``` %prep %forgesetup -a %forgeautosetup -z0 -p1 mv -T %{_builddir}/%{extractdir2} lib/libcommuni mv -T %{_builddir}/%{extractdir3} lib/settings mv -T %{_builddir}/%{extractdir4} lib/signals mv -T %{_builddir}/%{extractdir5} lib/serialize mv -T %{_builddir}/%{extractdir9} lib/magic_enum mv -T %{_builddir}/%{extractdir11} lib/certify mv -T %{_builddir}/%{extractdir12} lib/expected-lite ``` - You still have a reference to `%{uuid}` in the `%check` section, and a few other glob patterns. I am fine with the `uuid` macro btw, it's just the glob patterns that could hide some issues down the line.
Spec URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2.spec SRPM URL: https://solomoncyj.fedorapeople.org/reviews/chatterino2/chatterino2-2.5.4^20251110gitf355928-1.fc44.src.rpm - Uncommitted changes