https://bugzilla.redhat.com/show_bug.cgi?id=2315886 --- Comment #3 from Sandro <gui1ty@xxxxxxxxxxxxx> --- Thank you for the review and the feedback provided. Much appreciated. (In reply to Ben Beasley from comment #2) > - The License field does not appear to be correct. <snip> > # The entire source is BSD-2-Clause, except: > # BSL-1.0: > # - examples/valijson_nlohmann_bundled.hpp (removed in %%prep) > # - include/valijson/compat/optional.hpp > # Additionally, Source1 is MIT, but is used only for testing and does not > # contribute to the licenses of the binary RPMs. > License: BSD-2-Clause AND BSL-1.0 Not sure what I did there. I might have been looking at the test suite repo when determining the license. Either way, I corrected it now. > Then, to be strictly correct: > > # include/valijson/compat/optional.hpp is derived/forked from an unknown > # release of the Boost.Optional library > Provides: bundled(boost) This feels very strict seeing this is concerning a single header file. Is it even true? The package is not providing any binaries, nor libraries, etc.? I'd rather not add the Provides. I'd be happy to add a comment regarding that file, though. > - It’s good and correct that you added Requires to valijson-devel for things > that appear in the API headers. However, based on grepping the headers, I > think that > > Requires: cmake(gtest) > > is spurious and can be omitted. Done. It was a left over from copying the test BRs. > - Please replace > > pushd %{__cmake_builddir} > > with > > pushd %{_vpath_builddir} Changed. Thanks! > - Since this is a header-only library, you must add > > Provides: valijson-static = %{version}-%{release} Done. I remembered the debug_package part, but forgot about the static library part. > ===== Suggestions/Comments (no change required for approval) ===== > > - It would indeed be good to try to investigate the failing tests. I can’t > reproduce them in an upstream checkout: > > $ gh repo clone tristanpenman/valijson > $ cd valijson > $ git submodule update --init --recursive > $ cmake -S. -Bbuild -Dvalijson_BUILD_TESTS:BOOL=ON > $ cmake --build build/ -j16 --verbose > $ cd build > $ ./test_suite I agree. Though, my C++ skills are not very advanced. My gut tells me this has to do with some version difference of some library. Upstream is using git submodules after all. > - Personally, I like writing directories in files lists with trailing slashes > so that they only match directories and not files, and so that it is clear > to > the reader that a recursively-included directory is intended. For example, > instead of > > %{_includedir}/%{name} > %{_libdir}/cmake/%{name} > > I would write > > %{_includedir}/%{name}/ > %{_libdir}/cmake/%{name}/ > > However, there is no guideline prescribing this, and it is ultimately a > matter of opinion. I think it's a good habit of trying to avoid mismatches where possible. I changed it and will try to remember and make it a habit. -- 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=2315886 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202315886%23c3 -- _______________________________________________ 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
