-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[native] Update dockerfiles to use Centos9 #22909
Conversation
ca7bf98
to
fe9ab3b
Compare
fe9ab3b
to
20b0518
Compare
@@ -10,11 +10,11 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
FROM quay.io/centos/centos:stream8 | |||
FROM quay.io/centos/centos:stream9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should rename the file to centos-dependency.dockerfile to remove the version? This would match a similar change for the dockerfile in Velox from centos-8-stream.dockerfile
to centos.dockerfile
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this.
@majetideepak FYI. |
@@ -27,6 +27,7 @@ RUN mkdir -p /prestissimo /runtime-libraries | |||
COPY . /prestissimo/ | |||
RUN EXTRA_CMAKE_FLAGS=${EXTRA_CMAKE_FLAGS} \ | |||
make -j${NUM_THREADS} --directory="/prestissimo/" cmake-and-build BUILD_TYPE=${BUILD_TYPE} BUILD_DIR=${BUILD_DIR} BUILD_BASE_DIR=${BUILD_BASE_DIR} | |||
RUN echo "/usr/local/lib" >> /etc/ld.so.conf.d/prestissimo.conf && echo "/usr/local/lib64" >> /etc/ld.so.conf.d/prestissimo.conf && ldconfig | |||
RUN ldd /prestissimo/${BUILD_BASE_DIR}/${BUILD_DIR}/presto_cpp/main/presto_server | awk 'NF == 4 { system("cp " $3 " /runtime-libraries") }' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we should ideally avoid ldconfig since a package installation usually should not depend on it.
If we copy the libraries to the corresponding lib or lib64, will that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, we modify | awk 'NF == 4 { system("cp " $3 " /runtime-libraries") }'
to install to lib and lib64 and copy them out as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can try to copy them to /usr/lib
and see if that also fixes the problem.
00ab322
to
761d91d
Compare
@@ -27,7 +27,8 @@ RUN mkdir -p /prestissimo /runtime-libraries | |||
COPY . /prestissimo/ | |||
RUN EXTRA_CMAKE_FLAGS=${EXTRA_CMAKE_FLAGS} \ | |||
make -j${NUM_THREADS} --directory="/prestissimo/" cmake-and-build BUILD_TYPE=${BUILD_TYPE} BUILD_DIR=${BUILD_DIR} BUILD_BASE_DIR=${BUILD_BASE_DIR} | |||
RUN ldd /prestissimo/${BUILD_BASE_DIR}/${BUILD_DIR}/presto_cpp/main/presto_server | awk 'NF == 4 { system("cp " $3 " /runtime-libraries") }' | |||
RUN LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/usr/local/lib:/usr/local/lib64 ldd /prestissimo/${BUILD_BASE_DIR}/${BUILD_DIR}/presto_cpp/main/presto_server > /tmp/dynamic_libs.txt && ! grep "not found" /tmp/dynamic_libs.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can pipe to grep and check. No need to write to a file.
!(LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/usr/local/lib:/usr/local/lib64 ldd /prestissimo/${BUILD_BASE_DIR}/${BUILD_DIR}/presto_cpp/main/presto_server | grep "not found")
We should also combine both to avoid an extra RUN step.
I see some approaches here
RUN test -d /app/bin -a ... \
&& mkdir /app/control/bin
https://stackoverflow.com/questions/53096984/how-to-stop-building-when-i-use-dockerfile-build-image
Upgrade to use Centos9 as build and runtime.
761d91d
to
5174c4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @czentgr
Upgrade to use Centos9 as build and runtime.
Resolves: #22901
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.