-
Notifications
You must be signed in to change notification settings - Fork 71
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
Change CI to use published Docker image #357
Conversation
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!! That will help a lot for testing both locally and on CI.
RUN git clone https://github.com/tpm2-software/tpm2-tss.git --branch 2.3.3 | ||
RUN cd tpm2-tss \ | ||
&& ./bootstrap \ | ||
&& ./configure \ | ||
&& make -j$(nproc) \ | ||
&& make install \ | ||
&& ldconfig | ||
|
||
RUN rm -rf tpm2-tss |
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.
Would it work to use the Ubuntu package here instead?
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.
Hmm, I only searched for what's directly accessible from apt
(and it doesn't appear there), but maybe if I add the correct registry... Will try.
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.
Oh... The thing you linked is a source package - it just contains the source that you build from. Looks like those sources were used to compile libsapi
, so no esapi
. They also mention on their Install page that there's a package available for Fedora, but there's no mention of Ubuntu/Debian.
I was wondering if we should switch to testing on Fedora, I think there are a few dependencies that we could get for free.
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.
Worth to check but it's fine if the size is not going to be a lot smaller actually..
RUN cd SoftHSMv2 \ | ||
&& sh autogen.sh \ | ||
# Autogen might fail because of some limitations of autoconf, see: | ||
# https://bugzilla.redhat.com/show_bug.cgi?id=1826935#c3 | ||
|| sh autogen.sh | ||
RUN cd SoftHSMv2 \ | ||
&& ./configure --disable-gost \ | ||
&& make \ | ||
&& make install | ||
RUN rm -rf SoftHSMv2 |
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.
There is also a softhsm
package avaible
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.
And a softhsm2
but it was throwing some weird errors when used :( so I had to revert to this one
# and is found with the find_slot_number script. | ||
RUN softhsm2-util --init-token --slot 0 --label "Parsec Tests" --pin 123456 --so-pin 123456 | ||
|
||
# Install Rust toolchain for all users |
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.
Nice 👌
RUN apt install -y autoconf-archive libcmocka0 libcmocka-dev procps | ||
RUN apt install -y iproute2 build-essential git pkg-config gcc libtool automake libssl-dev uthash-dev doxygen libjson-c-dev | ||
RUN apt install -y --fix-missing wget python3 cmake clang | ||
RUN apt install -y gcc-multilib | ||
RUN apt install -y gcc-arm-linux-gnueabihf | ||
RUN apt install -y gcc-aarch64-linux-gnu | ||
RUN apt install -y libini-config-dev libcurl4-openssl-dev tpm2-tools curl libgcc1 |
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.
Did you by any chance check if all of these are actually needed 😬 ?
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.
Yes, I started from scratch - they're all needed 😞
|
||
# Add users for multitenancy tests | ||
RUN useradd -m parsec-client-1 | ||
RUN useradd -m parsec-client-2 |
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 a:
RUN apt autoremove
or something similar would slim the container even more?
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.
Just tried running it inside the built container - only removed about 60 MB of stuff. Will add it, though. I might try and look up which of the dependencies are no longer needed, maybe we can slim some of that too, though I think the big ones are needed (gcc, clang...). Do we need Python for any "runtime compilation" step? 🤔
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.
ah yes true, maybe some dependencies are only needed to build things but then not needed anymore? Python is heavily used by Ubuntu, last time I uninstalled it on my system, it destroyed everything 🔥
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.
Hmm, yeah, I've been prodding here and there, but don't think there's much space I can save.. :( The libraries that I can remove are small, around 1MB.
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.
Initially I had added this, but then things started failing because some useful libraries were being removed... So I've scrapped it again, but it's good to know that exists
|
||
# Install the wrappers for the Rust binaries installed earlier | ||
COPY _exec_wrapper /usr/local/bin/ | ||
RUN ls /opt/rust/bin | xargs -n1 -I% ln -s /usr/local/bin/_exec_wrapper /usr/local/bin/$(basename %) |
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.
What does this line actually does with _exec_wrapper
?
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.
Searches for each file in /opt/rust/bin
(e.g. rustc
) and creates a symlink that points at _exec_wrapper
. So you end up with, e.g., /usr/local/bin/rustc -> /usr/local/bin/_exec_wrapper
which then calls the real rustc
with all the args.
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.
oh that's cool!
5726b80
to
446d38c
Compare
Ok, build time dropped from 23 minutes to 12 (dominated by the I'll try to integrate the |
446d38c
to
3ad2c36
Compare
This commit modifies the CI to use a container image published to the Github container repository. The Dockerfile for the image is also included. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
3ad2c36
to
9335431
Compare
Ok, this PR is READY 🔥 I moved the Trusted Services stuff in the Ubuntu 18 image (which clocks in at around 4 GB for the final image - about 9 with all the precursors), everything in Github Actions is now using the same one! I've also added Trusted Service provider to coverage reporting! ☂️ |
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.
Looks good!! Thank you 😄
Would it be possible to host the image (or images in the future) in their own repo so that when a PR is merged, the image is automatically built and upstreamed? I am saying as we might have from time to time requests to update it, and it might be easier to maintain than doing it manually. But that can also be fine for now! |
This commit modifies the CI to use a container image published to the
Github container repository. The Dockerfile for the image is also
included.
Signed-off-by: Ionut Mihalcea ionut.mihalcea@arm.com