Skip to content
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.

Sign up for GitHub

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

add lws to image build #369

Merged
merged 2 commits into from
Apr 26, 2022
Merged

add lws to image build #369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a45fce
add lws to image build
ajssmith Apr 22, 2022
73143de
bump lws version
ajssmith Apr 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions .github/scripts/compile.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,37 @@ do_patch () {

WORKING=`pwd`
wget ${PROTON_SOURCE_URL} -O qpid-proton.tar.gz
wget ${LWS_SOURCE_URL} -O libwebsockets.tar.gz

mkdir -p qpid-proton-src build staging proton_build proton_install
mkdir -p qpid-proton-src build staging proton_build proton_install lws-src lws_build lws_install
tar -zxf qpid-proton.tar.gz -C qpid-proton-src --strip-components 1
tar -zxf libwebsockets.tar.gz -C lws-src --strip-components 1

do_patch "patches/proton" qpid-proton-src

cd proton_build
cd $WORKING/lws_build
cmake \
-D LWS_LINK_TESTAPPS_DYNAMIC=ON \
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor but just for consistency sake let's remove the space between the -D and the options.
E.g. instead of -D LWS_WITH_LIBUV=OFF, let's do -DLWS_WITH_LIBUV=OFF

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it more valuable to be consistent with libwebsockets.spec file or with other cmake builds in this dockerfile? :P

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like it if it were consistent with other builds in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

/me quietly withdraws from the thread

Copy link
Member Author

Choose a reason for hiding this comment

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

agree that we should make it consistent with other formats in the project

-D LWS_WITH_LIBUV=OFF \
-D LWS_WITHOUT_BUILTIN_GETIFADDRS=ON \
-D LWS_USE_BUNDLED_ZLIB=OFF \
-D LWS_WITHOUT_BUILTIN_SHA1=ON \
-D LWS_WITH_STATIC=OFF \
-D LWS_IPV6=ON \
-D LWS_WITH_HTTP2=OFF \
-D LWS_WITHOUT_CLIENT=OFF \
-D LWS_WITHOUT_SERVER=OFF \
-D LWS_WITHOUT_TESTAPPS=ON \
-D LWS_WITHOUT_TEST_SERVER=ON \
-D LWS_WITHOUT_TEST_SERVER_EXTPOLL=ON \
-D LWS_WITHOUT_TEST_PING=ON \
-D LWS_WITHOUT_TEST_CLIENT=ON \
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the logic for choosing what options to enable? Is it a list of all switches currently available, or is it carefully chosen?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing these options were chosen based off off the build log of LWS in brew.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be, the Rawhide version is https://src.fedoraproject.org/rpms/libwebsockets/blob/rawhide/f/libwebsockets.spec#_58

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at that rawhide link, it seems to me that the LWS_WITH_LIBUV option by default is OFF
You can see that in the LWS CMakeLists.txt
https://github.com/warmcat/libwebsockets/blob/main/CMakeLists.txt#L215

But in brew, this option is set to ON

Copy link
Member Author

Choose a reason for hiding this comment

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

I used what was here https://dist-git.host.prod.eng.bos.redhat.com/cgit/rpms/libwebsockets/ as a starting point and then turned LIBUV off

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, for lws 3.x, I think; for latest lws the reasonable defaults would be the fedora rawhide

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix formatting but will leave to a separate task option defaults as rawhide options bring in other dependencies

$WORKING/lws-src \
&& make \
&& make DESTDIR=$WORKING/lws_install install \
&& tar -z -C $WORKING/lws_install -cf /libwebsockets-image.tar.gz usr \
&& make install
cd $WORKING/proton_build
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DENABLE_LINKTIME_OPTIMIZATION=ON \
-DCMAKE_POLICY_DEFAULT_CMP0069=NEW -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON \
Expand Down
13 changes: 7 additions & 6 deletions Containerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,23 @@ FROM registry.access.redhat.com/ubi8/ubi:latest as builder
RUN dnf -y --setopt=tsflags=nodocs install \
http://mirror.centos.org/centos/8-stream/BaseOS/x86_64/os/Packages/centos-gpg-keys-8-4.el8.noarch.rpm \
http://mirror.centos.org/centos/8-stream/BaseOS/x86_64/os/Packages/centos-stream-repos-8-4.el8.noarch.rpm \
&& dnf -y --setopt=tsflags=nodocs install epel-release \
&& dnf config-manager --set-enabled powertools \
&& dnf clean all

RUN dnf -y --setopt=tsflags=nodocs install \
gcc gcc-c++ make cmake \
cyrus-sasl-devel openssl-devel libuuid-devel \
python3-devel swig \
libwebsockets-devel libnghttp2-devel \
libnghttp2-devel \
wget patch findutils git valgrind \
&& dnf clean all -y

WORKDIR /build
COPY . .
ENV PROTON_VERSION=0.37.0
ENV PROTON_SOURCE_URL=${PROTON_SOURCE_URL:-http://archive.apache.org/dist/qpid/proton/${PROTON_VERSION}/qpid-proton-${PROTON_VERSION}.tar.gz}
ENV LWS_VERSION=v3.2.3
Copy link
Contributor

Choose a reason for hiding this comment

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

somewhat OFFTOPIC: I wonder about the version choice. Epel had 4.0.1-2.el8 Why not use latest here? That would be v4.3.1, https://github.com/warmcat/libwebsockets/releases/tag/v4.3.1

Copy link
Contributor

Choose a reason for hiding this comment

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

Table of all versions in the opensource RH world, https://src.fedoraproject.org/rpms/libwebsockets

Copy link
Member Author

Choose a reason for hiding this comment

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

I am open to using latest, I just followed what was being used in the rpm build we had for previous release.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ganeshmurthy wdyt, also taking #48 into consideration. We don't have to resolve this on the PR right away, of course. Just something to keep in mind and eventually take care of.

Depending also on what @mcressman thinks, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am all for using the latest version of LWS

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update to v4.3.1

ENV LWS_SOURCE_URL=${LWS_SOURCE_URL:-https://github.com/warmcat/libwebsockets/archive/refs/tags/${LWS_VERSION}.tar.gz}

ARG VERSION=UNKNOWN
ENV VERSION=$VERSION
Expand All @@ -48,7 +50,6 @@ RUN dnf -y --setopt=tsflags=nodocs update \
&& dnf -y --setopt=tsflags=nodocs install \
http://mirror.centos.org/centos/8-stream/BaseOS/x86_64/os/Packages/centos-gpg-keys-8-4.el8.noarch.rpm \
http://mirror.centos.org/centos/8-stream/BaseOS/x86_64/os/Packages/centos-stream-repos-8-4.el8.noarch.rpm \
&& dnf -y --setopt=tsflags=nodocs install epel-release \
&& dnf config-manager --set-enabled powertools \
&& dnf clean all

Expand All @@ -57,14 +58,14 @@ RUN dnf -y --setopt=tsflags=nodocs install \
glibc \
cyrus-sasl-lib cyrus-sasl-plain cyrus-sasl-gssapi libuuid openssl \
python3 \
libwebsockets libnghttp2 \
libnghttp2 \
gdb valgrind \
gettext hostname iputils \
&& dnf clean all

WORKDIR /
COPY --from=builder /qpid-proton-image.tar.gz /skupper-router-image.tar.gz /
RUN tar zxpf qpid-proton-image.tar.gz && tar zxpf skupper-router-image.tar.gz && rm -f /qpid-proton-image.tar.gz /skupper-router-image.tar.gz
COPY --from=builder /qpid-proton-image.tar.gz /skupper-router-image.tar.gz /libwebsockets-image.tar.gz /
RUN tar zxpf qpid-proton-image.tar.gz && tar zxpf skupper-router-image.tar.gz && tar zxpf libwebsockets-image.tar.gz && rm -f /qpid-proton-image.tar.gz /skupper-router-image.tar.gz /libwebsockets-image.tar.gz

WORKDIR /home/skrouterd/etc
WORKDIR /home/skrouterd/bin
Expand Down