Skip to content

Conversation

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @brosenberg42)


components/cpp_component_build/Dockerfile, line 41 at r1 (raw file):

# which prevents other locales (including other English locales) from being installed with GCC.
RUN yum-config-manager --setopt=override_install_langs='' \
                       --setopt=skip_missing_names_on_install=False --save; \

Should we add --setopt=skip_missing_names_on_install=False to the activemq Dockerfile as well since that uses yum to install things? Also, the integration tests Dockerfile uses yum before it extends from openmpf_build.


components/cpp_component_build/Dockerfile, line 53 at r1 (raw file):

    rm --recursive /var/cache/yum/*; \
    ln --symbolic cuda-10.2 /usr/local/cuda; \
    # Opencv doesn't use the statically compiled CUDA libraries except for libcudart and they are relatively large.

Use caps: "OpenCV"


openmpf_build/Dockerfile, line 114 at r1 (raw file):

        -DBUILD_CUDA_STUBS=true \
        -DOPENCV_EXTRA_MODULES_PATH=/tmp/opencv-contrib/opencv_contrib-4.5.0/modules \
        -DOPENCV_ENABLE_NONFREE=true ..; \

Why do we now require OPENCV_ENABLE_NONFREE? From what I can gather, this enables support for the SIFT and SURF algorithms.

I ask because if we enable this setting we should update the license considerations language on the DockerHub overview pages to mention it, similar to how the language states that our images come with codecs that may require paying royalties if used commercially.

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 12 files reviewed, 2 unresolved discussions (waiting on @brosenberg42 and @jrobble)


components/cpp_component_build/Dockerfile, line 41 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Should we add --setopt=skip_missing_names_on_install=False to the activemq Dockerfile as well since that uses yum to install things? Also, the integration tests Dockerfile uses yum before it extends from openmpf_build.

Done.


openmpf_build/Dockerfile, line 114 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Why do we now require OPENCV_ENABLE_NONFREE? From what I can gather, this enables support for the SIFT and SURF algorithms.

I ask because if we enable this setting we should update the license considerations language on the DockerHub overview pages to mention it, similar to how the language states that our images come with codecs that may require paying royalties if used commercially.

That was in the build command I was given. I removed it.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r2, 13 of 13 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@brosenberg42 brosenberg42 merged commit 32d072c into develop Dec 16, 2020
@brosenberg42 brosenberg42 deleted the feature/opencv4 branch December 16, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants