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.

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

[MDBF-443] Create BuildBot worker image with MSAN #72

Merged
merged 1 commit into from Feb 9, 2023

Conversation

AlexPykavy
Copy link
Contributor

MSAN has been built according to the https://libcxx.llvm.org/BuildingLibcxx.html instructions

There is also an issue with debian source package llvm/llvm-project#59240 which leads to some warnings during libcxx build.

And I've run make install which moved built libraries and headers to the /usr/local. Is it fine or they have to reside somewhere else?

@AlexPykavy
Copy link
Contributor Author

And the build failed due to hadolint( but I've intentionally used cd operator to download sources, compile MSAN and cleanup in the single layer. May I just suppress it?

@fauust
Copy link
Collaborator

fauust commented Nov 29, 2022

And the build failed due to hadolint( but I've intentionally used cd operator to download sources, compile MSAN and cleanup in the single layer. May I just suppress it?

It's ok to disable that check just before the RUN command, we already disable some hadolint checks in other dockerfiles. I am not sure if that's applicable here but you could consider using a multi-stage building. In any case, thank you for trying to keep the image small.

@fauust
Copy link
Collaborator

fauust commented Nov 29, 2022

And I've run make install which moved built libraries and headers to the /usr/local. Is it fine or they have to reside somewhere else?

I suggest we follow MSAN default install/build docs, so, if that's the case, that's fine.

@AlexPykavy
Copy link
Contributor Author

I found a way to avoid using cd command by renaming the downloaded package directory and including it in the paths.

@vladbogo
Copy link
Collaborator

Regarding the install PATH, I guess it should be fine, but we need to test it out first. Thanks for the contribution!

image: ubuntu:22.04
branch: 10.7
tag: msanubuntu22
platforms: linux/amd64, linux/arm64/v8, linux/ppc64le, linux/s390x
Copy link
Collaborator

Choose a reason for hiding this comment

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

We only need MSAN for amd64

.github/workflows/bb_containers.yml Outdated Show resolved Hide resolved
@vladbogo
Copy link
Collaborator

vladbogo commented Dec 12, 2022

also, there are other libs that need to be instrumented in order not to prevent failures (see build-msan2.sh) in the Jira task:

#!/bin/sh
set -eux
: ${CLANG=10}
: ${MSAN_LIBDIR=/msan-libs}
: ${PARALLEL=$(nproc)}

cd gnutls28-*/
aclocal
automake --add-missing
./configure --with-included-libtasn1 --with-included-unistring \
	    --without-p11-kit --disable-hardware-acceleration -with-libnettle-prefix=/usr
make -j$PARALLEL
cd ..

cd nettle-*/
./configure --disable-assembler
make -j$PARALLEL
cd ..

cd libidn2-*/
./configure --enable-valgrind-tests=no
make -j$PARALLEL
cd ..

cd gmp-*/
./configure --disable-assembly
make -j$PARALLEL
cd ..

cd cracklib2-*/
./configure --with-default-dict=/usr/share/dict/cracklib-small
make -j$PARALLEL
make install
create-cracklib-dict /tmp/build/cracklib*/dicts/cracklib-small
cd ..

cp -aL llvm-toolchain-$CLANG-$CLANG*/libc++msan/lib/libc++.so* \
   gnutls28-*/lib/.libs/libgnutls.so* \
   nettle-*/.lib/lib*.so* \
   gmp-*/.libs/libgmp.so* \
   libidn2-*/lib/.libs/libidn2.so \
   cracklib2*/lib/.libs/*.so* \
   "$MSAN_LIBDIR"
 ```

@AlexPykavy
Copy link
Contributor Author

@grooverdan , thank you so much for your help in completing this task.

@grooverdan
Copy link
Member

Thanks @AlexPykavy for starting it and @vladbogo for pointing to how to finish it. Its built according to spec now, but I haven't tired it in action.

Copy link
Collaborator

@vladbogo vladbogo left a comment

Choose a reason for hiding this comment

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

LGTM, but based on experience probably some there will be some other small issues that need to be resolved

@fauust
Copy link
Collaborator

fauust commented Feb 7, 2023

@AlexPykavy after some time off I dumbly restarted this work without remembering that you already almost finished it (the msan builder came again in a slack discussion)!

Your PR is better since it uses upstream sources, but I think that it would be cleaner to put the script in the Dockerfile, I don't see any complex bash syntax that would be too difficult to maintain in a Dockerfile. Can you based on #72 integrate the bash script in the Dockerfile please.

Then I will comment directly in the code since I have found some small errors.

.github/workflows/bb_containers.yml Outdated Show resolved Hide resolved
ci_build_images/msan.Dockerfile Outdated Show resolved Hide resolved
ci_build_images/msan.Dockerfile Outdated Show resolved Hide resolved
ci_build_images/msan.Dockerfile Outdated Show resolved Hide resolved
@AlexPykavy
Copy link
Contributor Author

I've also changed the order a bit to sequentially download and extract each sources package to the current directory, build it, copy output to the MSAN_LIBDIR and clean up the directory. This is done to avoid using cd which was not recommended by hadolint.

@fauust
Copy link
Collaborator

fauust commented Feb 9, 2023

That's looking very good now! Thanks a lot @AlexPykavy for your contribution and sorry for the time it took to review your PR. I will now merge it since the prod use the tag ubuntu20.04-msan and this image will need extra testing before activation (probably next week).

@fauust fauust merged commit 64f42d4 into MariaDB:main Feb 9, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants