search

tpm2-software / tpm2-tss-engine

OpenSSL Engine for TPM2 devices
https://tpm2-software.github.io
BSD 3-Clause "New" or "Revised" License
151 stars 100 forks source link

Switch CI to Github Actions and Docker Images #197

Closed williamcroberts closed 3 years ago

williamcroberts commented 4 years ago

A few questions, in the .travis.yaml file, all gcc builds have coverage enabled: https://github.com/tpm2-software/tpm2-tss-engine/blob/ae070caf92b308ae81d63083784b0ca52222d748/.travis.yml#L104-L108

The matrix build is currently 2x3 yielding 6 builds, 3 of which are gcc. Does this mean you end up with 3 code coverage reports for each build, is that right? Do you want this variability? Things like changing OSSL version could cause code to be included/excluded, etc.

The next questions are: the build is on xenial (thats 16.04 right?), is that a requirement or just arbitrary? Do you know of things breaking offhand switching that out to a newer ubuntu version?

williamcroberts commented 4 years ago

Another question, can I move your CI commands into a separate script, it greatly simplifies things.

AndreasFuchsTPM commented 4 years ago

3 code coverage reports:

That's intentional, because we have #ifdefs for the different openssl API versions (unfortunately) and we want to report the total coverage over all branches

xenial

That's just what travis had at the time. Should be working on anything else too. I'd love to see the docker-stuff working for this as well

separate script

Sure, love to.

williamcroberts commented 4 years ago

@AndreasFuchsSIT thanks for the response. Okay, Ill flip over to the docker stuff.... let's see what I can get working for ya. Ill leave this ticket open and close it with a PR.

williamcroberts commented 3 years ago

@AndreasFuchsSIT the tests include failwrite.sh. However, this doesn't work in a root environment (root has all capabilities and is allowed to bypass DAC by default), is this test a hard requirement? Is this test not something that can be mocked better?

AndreasFuchsTPM commented 3 years ago

It was added to test graceful failure, but I guess you knew that...

I guess we could try to open /dev/null/doesnotexist instead ?

williamcroberts commented 3 years ago

It was added to test graceful failure, but I guess you knew that...

I guess we could try to open /dev/null/doesnotexist instead ?

That's not a bad idea. My other thought was to detect the caps and turn down cap dac override. But I like your idea better.