Bug 1413646

Summary: Review Request: librealsense - Cross-platform camera capture for Intel RealSense
Product: [Fedora] Fedora Reporter: Till Hofmann <thofmann>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: package-review, tcallawa
Target Milestone: ---Flags: mtasaka: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-04-03 16:09:03 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
CLA when contributing to this software none

Description Till Hofmann 2017-01-16 15:38:19 UTC
Spec URL: https://thofmann.fedorapeople.org/librealsense.spec
SRPM URL: https://thofmann.fedorapeople.org/librealsense-1.12.1-3.fc25.src.rpm
Description:
This project is a cross-platform library (Linux, OSX, Windows) for capturing
data from the Intel® RealSense™ F200, SR300 and R200 cameras. This effort was
initiated to better support researchers, creative coders, and app developers in
domains such as robotics, virtual reality, and the internet of things. Several
often-requested features of RealSense™ devices are implemented in this project,
including multi-camera capture.

Fedora Account System Username: thofmann
COPR: https://copr.fedorainfracloud.org/coprs/thofmann/realsense/

Comment 1 Mamoru TASAKA 2017-01-20 11:14:39 UTC
Some initial comments:

* License
  https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#How_should_I_handle_multiple_licensing_situations.3F
  - src/libuvc/ is under BSD and librealsense uses (links) files under
    this directory, so the license tag should be "ASL 2.0 and BSD"

* Compiler flags
  https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
  - Changing optimization level is discouraged. Currently -Ofast
    overrides -O2 in %optflags. e.g.

/usr/bin/c++   -DRS_USE_V4L2_BACKEND -DUNICODE -Drealsense_EXPORTS -isystem /usr/include/libusb-1.0 -I/builddir/build/BUILD/librealsense-1.12.1/include  -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -std=c++11 -fPIC -pedantic -g -Ofast -Wno-missing-field-initializers -Wno-switch -Wno-multichar -mssse3 -fPIC   -o CMakeFiles/realsense.dir/src/context.cpp.o -c /builddir/build/BUILD/librealsense-1.12.1/src/context.cpp

    If you really needs -Ofast, you need to write such reason on
    the spec file, otherwide please remove this.
    ! Note that -O3 enables some standard-NONcompliant optimizations
      such as -funsafe-math-optimizations and it is highly discouraged.

  - Also -mssse3 is added by default, this make it impossible to use
    this library on the platform not supporting ssse3, and to my
    understanding Fedora bans this type of changing supported 
    architecture.

* No Trademark
  https://fedoraproject.org/wiki/Packaging:Guidelines#Trademarks_in_Summary_or_Description
  - Remove ® ™ or so from %description

Comment 2 Mamoru TASAKA 2017-01-20 11:19:14 UTC
Created attachment 1242720 [details]
CLA when contributing to this software

Intel requests that to contribute librealsense (this package) via github pull requests (for example), the contributor is required to agree CLA attached.

Maybe this is regarded as some additional de-facto "license" for librealsense? If so, is this CLA allowed for Fedora package?

Comment 3 Till Hofmann 2017-01-21 14:59:06 UTC
Spec URL: https://thofmann.fedorapeople.org/librealsense.spec
SRPM URL: https://thofmann.fedorapeople.org/librealsense-1.12.1-4.fc25.src.rpm

Thank you for your comments!

(In reply to Mamoru TASAKA from comment #1)
> Some initial comments:
> 
> * License
>  
> https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/
> FAQ#How_should_I_handle_multiple_licensing_situations.3F
>   - src/libuvc/ is under BSD and librealsense uses (links) files under
>     this directory, so the license tag should be "ASL 2.0 and BSD"

Missed that, fixed.

> 
> * Compiler flags
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
>   - Changing optimization level is discouraged. Currently -Ofast
>     overrides -O2 in %optflags. e.g.
> 
> /usr/bin/c++   -DRS_USE_V4L2_BACKEND -DUNICODE -Drealsense_EXPORTS -isystem
> /usr/include/libusb-1.0 -I/builddir/build/BUILD/librealsense-1.12.1/include 
> -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2
> -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4
> -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64
> -mtune=generic -std=c++11 -fPIC -pedantic -g -Ofast
> -Wno-missing-field-initializers -Wno-switch -Wno-multichar -mssse3 -fPIC  
> -o CMakeFiles/realsense.dir/src/context.cpp.o -c
> /builddir/build/BUILD/librealsense-1.12.1/src/context.cpp
> 
>     If you really needs -Ofast, you need to write such reason on
>     the spec file, otherwide please remove this.
>     ! Note that -O3 enables some standard-NONcompliant optimizations
>       such as -funsafe-math-optimizations and it is highly discouraged.
> 
>   - Also -mssse3 is added by default, this make it impossible to use
>     this library on the platform not supporting ssse3, and to my
>     understanding Fedora bans this type of changing supported 
>     architecture.

I'm currently discussing this in https://github.com/IntelRealSense/librealsense/pull/422 whether they can add a flag that disables any modification of the compiler flags. If not, I'll stick with the new patch which simply removes all CFLAGS related stuff from CMakeLists (except the C++11 check).

> 
> * No Trademark
>  
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Trademarks_in_Summary_or_Description
>   - Remove ® ™ or so from %description

Oops, fixed.

I didn't think the CLA would be an issue as long as the code itself is still licensed under a good license.

But let's wait for FE-Legal on their opinion about the CLA.

Comment 4 Mamoru TASAKA 2017-01-30 04:49:38 UTC
Just for notes:

I also sent a question about this on legal mailing list but for now I got no response from spot.

Comment 5 Tom "spot" Callaway 2017-03-20 19:09:36 UTC
CLA does not impose additional restrictions on the license of librealsense, only on their acceptance of contributions to that codebase. 

Lifting FE-Legal.

Comment 6 Mamoru TASAKA 2017-03-21 03:36:10 UTC
Okay, thank you, spot.

(In reply to Till Hofmann from comment #3)
> Spec URL: https://thofmann.fedorapeople.org/librealsense.spec
> SRPM URL:
> https://thofmann.fedorapeople.org/librealsense-1.12.1-4.fc25.src.rpm
> 

Does not build on either F-27 or F-26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=18498661
https://koji.fedoraproject.org/koji/taskinfo?taskID=18498684

BTW I forgot to mention that now BR: gcc is needed:
https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B

Will review again when build issue on F-27 and F-26 is fixed.

Comment 7 Till Hofmann 2017-03-22 13:46:21 UTC
Spec URL: https://thofmann.fedorapeople.org/librealsense.spec
SRPM: https://thofmann.fedorapeople.org/librealsense-1.12.1-6.fc25.src.rpm 

Thank you spot for the clarification on the legal question.

I fixed the f27 and f26 builds and submitted a PR upstream: https://github.com/IntelRealSense/librealsense/pull/461.

I also added a BR: gcc-c++

builds:
rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=18520810
f26: https://koji.fedoraproject.org/koji/taskinfo?taskID=18520820

Comment 9 Mamoru TASAKA 2017-03-27 07:45:42 UTC
For -7:
* Trademarks
  - One "TM" is left.
    > often-requested features of >RealSense™< devices are...

Please fix the above when importing this package into Fedora git.

---------------------------------------------------------------
    This package (librealsense) is APPROVED by mtasaka
---------------------------------------------------------------

Comment 10 Gwyn Ciesla 2017-03-28 18:53:07 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/librealsense

Comment 11 Till Hofmann 2017-03-30 10:36:29 UTC
Thank you for reviewing!

Comment 12 Fedora Update System 2017-03-30 10:37:17 UTC
librealsense-1.12.1-7.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-c344f1e5bf

Comment 13 Fedora Update System 2017-03-30 10:37:27 UTC
librealsense-1.12.1-7.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-20f9581258

Comment 14 Fedora Update System 2017-03-30 10:37:33 UTC
librealsense-1.12.1-7.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-bc53957ca9

Comment 15 Fedora Update System 2017-03-30 14:39:28 UTC
ngspice-26-8.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-682cd9d476

Comment 16 Fedora Update System 2017-03-30 14:40:14 UTC
ngspice-26-8.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-10f4ed1314

Comment 17 Fedora Update System 2017-03-30 14:40:24 UTC
ngspice-26-8.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-2b71902776

Comment 18 Mamoru TASAKA 2017-03-30 14:42:26 UTC
(In reply to Fedora Update System from comment #15)
> ngspice-26-8.fc26 has been submitted as an update to Fedora 26.
> https://bodhi.fedoraproject.org/updates/FEDORA-2017-682cd9d476

(In reply to Fedora Update System from comment #16)
> ngspice-26-8.fc25 has been submitted as an update to Fedora 25.
> https://bodhi.fedoraproject.org/updates/FEDORA-2017-10f4ed1314

(In reply to Fedora Update System from comment #17)
> ngspice-26-8.fc24 has been submitted as an update to Fedora 24.
> https://bodhi.fedoraproject.org/updates/FEDORA-2017-2b71902776

Referenced wrong bug ticket, sorry.

Comment 19 Fedora Update System 2017-03-30 18:53:35 UTC
librealsense-1.12.1-7.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-c344f1e5bf

Comment 20 Fedora Update System 2017-03-31 03:19:49 UTC
librealsense-1.12.1-7.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-bc53957ca9

Comment 21 Fedora Update System 2017-03-31 03:22:04 UTC
librealsense-1.12.1-7.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-20f9581258

Comment 22 Fedora Update System 2017-04-03 16:09:03 UTC
librealsense-1.12.1-7.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2017-04-07 16:48:44 UTC
librealsense-1.12.1-7.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2017-04-07 18:19:36 UTC
librealsense-1.12.1-7.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.