https://bugzilla.redhat.com/show_bug.cgi?id=2439304

Cristian Le <[email protected]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[email protected]



--- Comment #3 from Cristian Le <[email protected]> ---
Hi Steve, I was around the discussions of the project and if you take it, just
want to point out a few major points that I want to raise about this:
- The Apache-2.0 license is not the only license of the project. See issue [1]
where this was highlighted
- Upstream is quite unresponsive and there were multiple forks and attempts to
help [2-4]. I would suggest to have a copr repo to review that this and the
metis update are actually compatible with each other
  @Antonio are you able to get in touch with upstream and get any communication
channel established to actually be able to contribute much of these changes. I
have offered my help to review the CMake parts and others in the community are
eager to help.

Otherwise, I do have some general packaging suggestions:
- (non-blocking) consider using %forgemeta macros [5]. It abstracts much of the
commit and such logic, and makes it much easier to move from a tagged to a
commit snapshot
- Please avoid picking up a dependency like valgrind-devel
- Please make the `sed` operation a patch. Proper patch is to make it use
GNUInstallDirs variables
- Please remove the following variables passed `CMAKE_VERBOSE_MAKEFILE`,
`CMAKE_INSTALL_PREFIX`. These are handled by the %cmake macro
- Consider using the %conf section if you do not need to backport this to too
old epels
- Be careful with the snippet
  ```
  %ifnarch %{?x86}
   -DNO_X86:BOOL=ON
  %endif
  ```
  whitespace/blank lines is important and it might not be handled well on i686
in that form
- The `PCRE_LDFLAGS=-lpcreposix` is very suspicious. Please elaborate on it.
Likely upstream should do a `find_package(PCRE)` instead and specify if they
want PCRE2
- `%undefine _ld_as_needed` is also suspicious
- Please abstract the soversion in a variable

[1]: https://github.com/KarypisLab/GKlib/issues/30
[2]: https://github.com/KarypisLab/GKlib/pull/34
[3]: https://github.com/KarypisLab/GKlib/pull/33
[4]: https://github.com/KarypisLab/GKlib/pull/28
[5]: example on my package
https://src.fedoraproject.org/rpms/span/blob/rawhide/f/span.spec


-- 
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=2439304

Report this comment as SPAM: 
https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202439304%23c3

-- 
_______________________________________________
package-review mailing list -- [email protected]
To unsubscribe send an email to [email protected]
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/[email protected]
Do not reply to spam, report it: 
https://forge.fedoraproject.org/infra/tickets/issues/new

Reply via email to