-
Notifications
You must be signed in to change notification settings - Fork 202
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
[tests,ci] Allow to build only tests #1913
base: master
Are you sure you want to change the base?
Conversation
34d086c
to
f30f3bc
Compare
Signed-off-by: Mariusz Zaborski <oshogbo@invisiblethingslab.com>
f30f3bc
to
d517dc7
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.
Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @oshogbo and @woju)
Suggestion:
[tests,CI]
meson.build
line 239 at r1 (raw file):
tomlc99_proj = subproject('tomlc99-208203af46bdbdb29ba199660ed78d09c220b6c5') tomlc99_dep = tomlc99_proj.get_variable('tomlc99_dep') tomlc99_src = tomlc99_proj.get_variable('tomlc99_src')
tabs -> spaces
libos/meson.build
line 22 at r1 (raw file):
if src_build subdir('include') subdir('src')
wrong indent
ditto for the whole PR, plz fix the whitespaces
!TODO: Use this commit msg: [tests,CI] Allow to build only tests Signed-off-by: Mariusz Zaborski <oshogbo@invisiblethingslab.com>
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.
Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @mkow and @woju)
-- commits
line 2 at r1:
Done.
meson.build
line 239 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
tabs -> spaces
Done.
libos/meson.build
line 22 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
wrong indent
ditto for the whole PR, plz fix the whitespaces
Oh, thank you for noticing.
I also fixed the ident around (add_project_arguments
which was added with C23) as I don't think we want to have another PR only with that.
@oshogbo We tested this PR and build gramine in similar way as mentioned in Error:
I have attached complete log I also tried with |
Signed-off-by: Mariusz Zaborski <oshogbo@invisiblethingslab.com>
@anjalirai-intel Thank you for reporting. This update should fix your issue. |
It fixed the gramine build installation. But in order to test it, I installed gramine from sources and tried to run pal/regression tests with Gramine-SGX. Error
Is this the correct way to test it? If not, then how should we test this PR functionality? |
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.
You can use an example from here.
Before this commit, gramine-test
had a hardcoded value indicating whether it was built with SGX support. This was embedded during the build process. In this PR, I changed that so gramine-test
searches for @PKGLIBDIR@/sgx/libpal.so
before each execution. However, the @PKGLIBDIR@
is still embedded from the build process. If your --prefix
differs from the actual installed gramine, then gramine-test
won't be able to find libpal
.
So, there are couple solutions:
- Leave it as it is, assuming that
gramine-test
will use the same--prefix
as the actual build (which was sufficient for my use case, linked above). - Change the newly added option
force_sgx_test
(which mandates building also tests that require SGX, like attestation) tosgx_test_pal
, where you can override the location of pals forgramine-test
. When provided, the SGX tests will also be built. - Keep
force_sgx_test
and add another optionsgx_test_pal
as described, without enforcing the build of SGX tests. - Remove this check, as it is only a developer tool.
Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @woju)
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.
Yeah, I think you can ignore 4th option. As we still need some path to libpal... But we could for example fetch them from environment variables or so.
Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @woju)
@oshogbo I took your suggestion and taken the reference from 1914 and build gramine with Observation 1: pal tests builds(gramine-test --sgx build) are getting failed with error Log:
Error:
Observation 2: fs, abi tests are successful. There are couple of failures with libos regression tests, listed below.
Log: You can ignore test_310_socket_tcp_ipv6_v6only test failure from log file I have attached the console logs for both pal tests and libos tests |
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.
@anjalirai-intel Regarding your Observation 2:
The failing tests were added/modified in very recent commits of Gramine. So my assumption is that you have differing versions of installed Gramine and of the installed Tests for Gramine (this PR). Please double-check that you build/install the exact same versions.
(Recall that Gramine internal tests like PAL, LibOS, FS, LTP are tied to a particular commit of Gramine. In other words, these tests are not supposed to be workable on a release version of Gramine, but only on the exact same commit version.)
Reviewed 4 of 8 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow, @oshogbo, and @woju)
-- commits
line 3 at r4:
I miss the rationale for this. Could you add in the commit body that this is useful to run the test suite on a Gramine version that was installed on the system by other means (e.g. via a package or Docker)?
meson.build
line 53 at r4 (raw file):
enable_libgomp = get_option('libgomp') == 'enabled' enable_tests = get_option('tests') == 'enabled' force_sgx_tests = get_option('force_sgx_tests') == 'enabled'
I think this can be removed, see my other comment
meson.build
line 244 at r4 (raw file):
endif mbedtls_proj = subproject('mbedtls-3.6.0')
I think you need an explanation comment why we build mbedTLS even in no-source mode: IIUC that's because some tests (like attestation tests) use mbedTLS crypto.
meson.build
line 305 at r4 (raw file):
if src_build subdir('tools') endif
Looking at this ugliness with paths, maybe it's time to move all our tests into a https://github.com/gramineproject/gramine/tree/master/tests dir? Then we'll have a much simpler logic with these conditionals...
.ci/linux-tests-only.jenkinsfile
line 1 at r4 (raw file):
node('nonsgx_slave && aesni') {
Why would we care about aesni
?
.ci/linux-tests-only.jenkinsfile
line 12 at r4 (raw file):
load '.ci/lib/config-clang.jenkinsfile' load '.ci/lib/stage-build-test-only.jenkinsfile'
This only tests that the build & install are successful. Why not test also the run of the tests?
libos/test/regression/meson.build
line 195 at r4 (raw file):
endif if sgx or force_sgx_tests
Actually, do you know what was the point of hiding this attestation
test under the if sgx
condition?
It looks to me that we can just remove the if
completely and unconditionally build this test. Then we can remove this force_sgx_tests
feature, right?
pal/meson.build
line 6 at r4 (raw file):
if enable_tests subdir('regression')
Why are PAL regression tests hidden under src_build
? Why can't we test them with this PR?
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
foreach output : mbedtls_libs_output meson.add_install_script('/bin/sh', '-c', ('mkdir -p "$MESON_INSTALL_DESTDIR_PREFIX"/@1@/gramine/runtime/@2@/i && ' +
What is this?
!TODO: Use this commit msg: [tests,CI] Allow to build only tests This commit allows the test suite to run on a Gramine version that was installed on the system by other means (e.g., via a package or Docker). Signed-off-by: Mariusz Zaborski <oshogbo@invisiblethingslab.com>
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.
Reviewable status: 5 of 9 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @anjalirai-intel, @dimakuv, @mkow, and @woju)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I miss the rationale for this. Could you add in the commit body that this is useful to run the test suite on a Gramine version that was installed on the system by other means (e.g. via a package or Docker)?
Ofc.
meson.build
line 53 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think this can be removed, see my other comment
Done.
meson.build
line 244 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think you need an explanation comment why we build mbedTLS even in no-source mode: IIUC that's because some tests (like attestation tests) use mbedTLS crypto.
Done.
meson.build
line 305 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Looking at this ugliness with paths, maybe it's time to move all our tests into a https://github.com/gramineproject/gramine/tree/master/tests dir? Then we'll have a much simpler logic with these conditionals...
Dunno, up to you guys. As this seems a separate issue, I would suggest doing it in a separate PR (I can do that), but up to you.
.ci/linux-tests-only.jenkinsfile
line 1 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why would we care about
aesni
?
My mistake.
.ci/linux-tests-only.jenkinsfile
line 12 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This only tests that the build & install are successful. Why not test also the run of the tests?
I didn't have a good plan for how to run these tests.
Should we install upstream gamine? Then it will break if test changes.
We might build tests separately and then build gramine without tests and merge them. Do we run only direct tests or sgx and direct? Doesn't it extend the time of the tests too much?
I figured out at this point we simply wanted to test and check if somebody didn't break Mason build files.
libos/test/regression/meson.build
line 195 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, do you know what was the point of hiding this
attestation
test under theif sgx
condition?It looks to me that we can just remove the
if
completely and unconditionally build this test. Then we can remove thisforce_sgx_tests
feature, right?
I don't know. I can only guess that somebody decided that without gramine-sgx
it doesn't make sense to test attestation.
I remove it.
pal/meson.build
line 6 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why are PAL regression tests hidden under
src_build
? Why can't we test them with this PR?
Test PAL requires a specific PAL:
See pal/regression/meson.build
, because we don't select any we don't know what to link to.
if skeleton
libpal = libpal_skeleton_dep
elif direct
libpal = libpal_direct_dep
elif sgx
libpal = libpal_sgx_dep
else
error('need to link tests against a PAL library, but no PAL version is enabled')
endif
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What is this?
@anjalirai-intel has reported it:
Running custom install script '/bin/sh -c ln -sf ../../../libmbedcrypto_gramine.so.16 "$MESON_INSTALL_DESTDIR_PREFIX"/lib/x86_64-linux-gnu/gramine/runtime/glibc/'
ln: failed to create symbolic link '/home/intel/anjali/gramine_1913/gramine_install/lib/x86_64-linux-gnu/gramine/runtime/glibc/': No such file or directory
FAILED: install script '/bin/sh -c ln -sf ../../../libmbedcrypto_gramine.so.16 "$MESON_INSTALL_DESTDIR_PREFIX"/lib/x86_64-linux-gnu/gramine/runtime/glibc/' exit code 1, stopped
FAILED: meson-internal__install
/usr/local/bin/meson install --no-rebuild
ninja: build stopped: subcommand failed.
The mbedtls expected that glbic directory would be created for it, as we don't install glibc, the directory is missing. It happens when you install a test in a different directory then gramine.
Jenkins, test Jenkins-SGX-EDMM please |
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.
Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @anjalirai-intel, @mkow, @oshogbo, and @woju)
meson.build
line 305 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Dunno, up to you guys. As this seems a separate issue, I would suggest doing it in a separate PR (I can do that), but up to you.
Yes, just throwing this out. This should be done (if ever) in a separate PR, yeah.
meson.build
line 243 at r5 (raw file):
endif # Build Mbed TLS because some tests, such as attestation tests, require it.
Better:
# Build Mbed TLS even if !src_build because some tests (attestation tests) require it.
.ci/linux-tests-only.jenkinsfile
line 12 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I didn't have a good plan for how to run these tests.
Should we install upstream gamine? Then it will break if test changes.
We might build tests separately and then build gramine without tests and merge them. Do we run only direct tests or sgx and direct? Doesn't it extend the time of the tests too much?
I figured out at this point we simply wanted to test and check if somebody didn't break Mason build files.
I think it's enough to:
- Build tests separately (
stage-build-test-only.jenkinsfile
) - Build Gramine separately, of the same version (
stage-build-gramine-only.jenkinsfile
, or maybe there is already such Jenkinsfile) - Run only direct tests.
Alternatively, I'll be fine if you just add a TODO comment here, explaining that currently this CI only builds the tests but doesn't verify they can run.
libos/test/regression/meson.build
line 195 at r4 (raw file):
I can only guess that somebody decided that without
gramine-sgx
it doesn't make sense to test attestation.
Yes, I think that was the reasoning. But there's nothing bad at building these files (even if never used) -- this saves as that ugliness with introducing force_sgx_tests
.
pal/meson.build
line 6 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Test PAL requires a specific PAL:
Seepal/regression/meson.build
, because we don't select any we don't know what to link to.if skeleton libpal = libpal_skeleton_dep elif direct libpal = libpal_direct_dep elif sgx libpal = libpal_sgx_dep else error('need to link tests against a PAL library, but no PAL version is enabled') endif
Ah, good point. Ok, resolving.
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
@anjalirai-intel has reported it:
Running custom install script '/bin/sh -c ln -sf ../../../libmbedcrypto_gramine.so.16 "$MESON_INSTALL_DESTDIR_PREFIX"/lib/x86_64-linux-gnu/gramine/runtime/glibc/' ln: failed to create symbolic link '/home/intel/anjali/gramine_1913/gramine_install/lib/x86_64-linux-gnu/gramine/runtime/glibc/': No such file or directory FAILED: install script '/bin/sh -c ln -sf ../../../libmbedcrypto_gramine.so.16 "$MESON_INSTALL_DESTDIR_PREFIX"/lib/x86_64-linux-gnu/gramine/runtime/glibc/' exit code 1, stopped FAILED: meson-internal__install /usr/local/bin/meson install --no-rebuild ninja: build stopped: subcommand failed.
The mbedtls expected that glbic directory would be created for it, as we don't install glibc, the directory is missing. It happens when you install a test in a different directory then gramine.
I see. This ln -s ...
is actually useless in this particular case (installing tests without Gramine), and also installing tests into a different directory than Gramine is wrong.
@oshogbo Could you check the existence of e.g. $MESON_INSTALL_DESTDIR_PREFIX"/lib/x86_64-linux-gnu/gramine/
directory in the top meson.build
file in case if not src_build
, and print an error message that "You're building only tests. They should be installed in the same directory as the rest of Gramine was installed (probed %s but didn't find Gramine there)."
@dimakuv yes, I looked some of the failed tests are from |
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.
one particular test
test_095_mkfifo
failed withstat error: Permission denied
This one was modified recently to add the stat()
syscall. So yes, this test can also be considered "new".
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @anjalirai-intel, @mkow, @oshogbo, and @woju)
Signed-off-by: Mariusz Zaborski <oshogbo@invisiblethingslab.com>
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.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @anjalirai-intel, @dimakuv, @mkow, and @woju)
meson.build
line 243 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Better:
# Build Mbed TLS even if !src_build because some tests (attestation tests) require it.
Done.
.ci/linux-tests-only.jenkinsfile
line 12 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think it's enough to:
- Build tests separately (
stage-build-test-only.jenkinsfile
)- Build Gramine separately, of the same version (
stage-build-gramine-only.jenkinsfile
, or maybe there is already such Jenkinsfile)- Run only direct tests.
Alternatively, I'll be fine if you just add a TODO comment here, explaining that currently this CI only builds the tests but doesn't verify they can run.
I will add the comment for know. When this pipeline will be active, I will revisit this issue.
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I see. This
ln -s ...
is actually useless in this particular case (installing tests without Gramine), and also installing tests into a different directory than Gramine is wrong.@oshogbo Could you check the existence of e.g.
$MESON_INSTALL_DESTDIR_PREFIX"/lib/x86_64-linux-gnu/gramine/
directory in the topmeson.build
file in caseif not src_build
, and print an error message that "You're building only tests. They should be installed in the same directory as the rest of Gramine was installed (probed %s but didn't find Gramine there)."
I was trying to do that, but I don't know how. I wanted to add this only to an install target; otherwise, you can't build tests without installing Gramine (for example, in CI).
There is no way to check if we are in the build/install step in Meson, and the add_install_script is executed after all files are installed.
So, do you have any idea how to do this?
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.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anjalirai-intel, @mkow, and @woju)
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I was trying to do that, but I don't know how. I wanted to add this only to an install target; otherwise, you can't build tests without installing Gramine (for example, in CI).
There is no way to check if we are in the build/install step in Meson, and the add_install_script is executed after all files are installed.
So, do you have any idea how to do this?
Well, can't we do this?
if get_option('libc') == 'glibc' and host_has_glibc and src_build
foreach output : mbedtls_libs_output
meson.add_install_script('/bin/sh', '-c', ...)
endforeach
endif
Note the and src_build
. Thus we'll invoke this install script only if we build Gramine itself.
You'll need to add src_build
as the "inherited from main meson project variable", by putting it here: https://github.com/gramineproject/gramine/blob/master/subprojects/packagefiles/mbedtls/meson_options.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.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anjalirai-intel, @mkow, and @woju)
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Well, can't we do this?
if get_option('libc') == 'glibc' and host_has_glibc and src_build foreach output : mbedtls_libs_output meson.add_install_script('/bin/sh', '-c', ...) endforeach endif
Note the
and src_build
. Thus we'll invoke this install script only if we build Gramine itself.You'll need to add
src_build
as the "inherited from main meson project variable", by putting it here: https://github.com/gramineproject/gramine/blob/master/subprojects/packagefiles/mbedtls/meson_options.txt
Not sure what foreach
changes (I have not tested your proposal).
However, when I added an install_script it was executed after the files were installed:
[0/1] Installing files.
Installing subdir /home/oshogbo/gramine/subprojects/mbedtls-3.6.0/mbedtls-3.6.0/include/mbedtls to /tmp/x/include/gramine/mbedtls
Installing /home/oshogbo/gramine/subprojects/mbedtls-3.6.0/mbedtls-3.6.0/include/mbedtls/md5.h to /tmp/x/include/gramine/mbedtls
Installing /home/oshogbo/gramine/subprojects/mbedtls-3.6.0/mbedtls-3.6.0/include/mbedtls/entropy.h to /tmp/x/include/gramine/mbedtls
Installing /home/oshogbo/gramine/subprojects/mbedtls-3.6.0/mbedtls-3.6.0/include/mbedtls/ecp.h to /tmp/x/include/gramine/mbedtls
Installing /home/oshogbo/gramine/subprojects/mbedtls-3.6.0/mbedtls-3.6.0/include/mbedtls/ecdh.h to /tmp/x/include/gramine/mbedtls
Installing /home/oshogbo/gramine/subprojects/mbedtls-3.6.0/mbedtls-3.6.0/include/mbedtls/pkcs12.h to /tmp/x/include/gramine/mbedtls
[...]
Installing libos/test/abi/x86_64/atexit_func to /tmp/x/lib/x86_64-linux-gnu/gramine/tests/libos/entrypoint
Installing libos/test/abi/x86_64/fpu_control_word to /tmp/x/lib/x86_64-linux-gnu/gramine/tests/libos/entrypoint
Installing libos/test/abi/x86_64/mxcsr to /tmp/x/lib/x86_64-linux-gnu/gramine/tests/libos/entrypoint
Installing libos/test/abi/x86_64/rflags to /tmp/x/lib/x86_64-linux-gnu/gramine/tests/libos/entrypoint
Installing libos/test/abi/x86_64/stack to /tmp/x/lib/x86_64-linux-gnu/gramine/tests/libos/entrypoint
Installing libos/test/abi/x86_64/stack_arg to /tmp/x/lib/x86_64-linux-gnu/gramine/tests/libos/entrypoint
[...]
Running custom install script '/bin/sh -c \n GRAMINE_DIR="$MESON_INSTALL_DESTDIR_PREFIX/lib/x86_64-linux-gnu/gramine/"\n if [ ! -d "$GRAMINE_DIR" ]; then\n
echo "You\'re installing only tests. " \\\n "They should be installed in the same directory as the rest of " \\\n "Gramine was installed (p
robed $GRAMINE_DIR but didn\'t find " \\\n\t "Gramine there)."\n\texit 1\n fi\n '
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.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anjalirai-intel, @mkow, and @woju)
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
Not sure what
foreach
changes (I have not tested your proposal).
? foreach
is already in the snippet. Here's the diff of what I propose:
- if get_option('libc') == 'glibc' and host_has_glibc
+ if get_option('libc') == 'glibc' and host_has_glibc and src_build
foreach output : mbedtls_libs_output
meson.add_install_script('/bin/sh', '-c',
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.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anjalirai-intel, @mkow, and @woju)
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not sure what
foreach
changes (I have not tested your proposal).?
foreach
is already in the snippet. Here's the diff of what I propose:- if get_option('libc') == 'glibc' and host_has_glibc + if get_option('libc') == 'glibc' and host_has_glibc and src_build foreach output : mbedtls_libs_output meson.add_install_script('/bin/sh', '-c',
Oh sorry, my bad...
I thought we were still talking about generating an error.
Yes, we can do that.
Signed-off-by: Mariusz Zaborski <oshogbo@invisiblethingslab.com>
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.
Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @anjalirai-intel, @dimakuv, @mkow, and @woju)
subprojects/packagefiles/mbedtls/meson.build
line 64 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Oh sorry, my bad...
I thought we were still talking about generating an error.
Yes, we can do that.
Done.
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.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @woju)
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.
Reviewed 1 of 8 files at r1, 1 of 1 files at r2, 2 of 3 files at r3, 1 of 4 files at r5, 2 of 3 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @woju)
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.
Reviewed 1 of 3 files at r3, 2 of 4 files at r5, 2 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @oshogbo and @woju)
.ci/linux-tests-only.jenkinsfile
line 7 at r7 (raw file):
docker.build( "local:${env.BUILD_TAG}", '-f .ci/ubuntu20.04.dockerfile .'
please rebase and update to 24.04
.ci/linux-tests-only.jenkinsfile
line 8 at r7 (raw file):
"local:${env.BUILD_TAG}", '-f .ci/ubuntu20.04.dockerfile .' ).inside("${env.DOCKER_ARGS_COMMON} --security-opt seccomp=${env.WORKSPACE}/scripts/docker_seccomp_mar_2021.json") {
same here
.ci/linux-tests-only.jenkinsfile
line 13 at r7 (raw file):
load '.ci/lib/stage-build-test-only.jenkinsfile' # XXX: we should also run test
That's not really possible? We didn't build anything except the tests.
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.
Reviewed 1 of 8 files at r1, 1 of 1 files at r2, 2 of 3 files at r3, 2 of 4 files at r5, 2 of 3 files at r6, 1 of 1 files at r7.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)
a discussion (no related file):
After you rebase against current master, please ping me and I'll add another pipeline to Jenkins.
meson.build
line 249 at r7 (raw file):
mbedtls_pal_dep = mbedtls_proj.get_variable('mbedtls_pal_dep') if src_build
Is it possible to fold this if
into the one before mbedtls_*
(or that other if
into here), so we'd have one if
branch less? I get we can't remove them all, but this constant if src_build
is kinda annoying.
meson.build
line 308 at r7 (raw file):
endif if src_build
Is this one true? Can't we build libcs without src_build
?
.ci/linux-tests-only.jenkinsfile
line 13 at r7 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
That's not really possible? We didn't build anything except the tests.
If it wasn't really possible, then this whole PR would be for nothing. Rationale provided in PR description was to run the test suite against gramine from packages. We can have a dockerfile that got those packages installed, or maybe we could even the official docker image from docker hub? Either way, we probably should run some tests. So either we fix it right now, or declare out of scope in this PR and keep this TODO (then plz. change XXX to TODO).
python/graminelibos/__init__.py
line 9 at r7 (raw file):
_CONFIG_LIBDIR = '@LIBDIR@' _CONFIG_SYSLIBDIR = '@SYSLIBDIR@' _CONFIG_SGX_ENABLED = _os.path.exists('@PKGLIBDIR@/sgx/libpal.so')
You can remove SGX_ENABLED
from meson.build
.
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.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @oshogbo and @woju)
.ci/linux-tests-only.jenkinsfile
line 13 at r7 (raw file):
If it wasn't really possible, then this whole PR would be for nothing.
I don't understand, this PR is clearly for allowing to build tests only and it does it. There's nothing about running them in CI without the rest of Gramine.
Rationale provided in PR description was to run the test suite against gramine from packages.
Not in CI? For manual tests.
We can have a dockerfile that got those packages installed, or maybe we could even the official docker image from docker hub?
We can't, because we would be testing latest Gramine relrease against tests from master
, that doesn't work because the tests include regression tests for stuff fixed after the release.
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.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)
.ci/linux-tests-only.jenkinsfile
line 13 at r7 (raw file):
We can't, because we would be testing latest Gramine relrease
That's true, I retract.
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.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)
.ci/linux-tests-only.jenkinsfile
line 13 at r7 (raw file):
We can't, because we would be testing latest Gramine release
In theory: we can build tests without Gramine, install them, then build Gramine without tests, and install it.
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.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @oshogbo and @woju)
.ci/linux-tests-only.jenkinsfile
line 13 at r7 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
We can't, because we would be testing latest Gramine release
In theory: we can build tests without Gramine, install them, then build Gramine without tests, and install it.
That sounds like an overkill, I don't think we want to ever do this? It would waste a lot of CI time.
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @oshogbo)
.ci/linux-tests-only.jenkinsfile
line 13 at r7 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
That sounds like an overkill, I don't think we want to ever do this? It would waste a lot of CI time.
Also wouldn't gain much I believe in addition to regular pipelines which do compile gramine and tests. We can skip testing in this pipeline.
Description of the changes
Allow to build only test. This can be useful for testing already released packages, so users can download the source tree and manually build only tests.
How to test this PR?
I have tried to add CI/CD, but not sure if it works.
This change is