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

Jump to bottom

libteeacl: condition libteeacl with WITH_TEEACL #336

Merged
merged 2 commits into from
Dec 8, 2022
Merged

libteeacl: condition libteeacl with WITH_TEEACL #336

merged 2 commits into from
Dec 8, 2022

Conversation

etienne-lms
Copy link
Contributor

Build and embed libteeacl upon WITH_TEEACL=1 (default configuration). This configuration switch allows one to build OP-TEE client without dependencies on pkg-config and libuuid when OP-TEE ACL for PKCS11 is not needed:
cmake -DWITH_TEEACL=0 ...
or
make WITH_TEEACL=0 ...

Signed-off-by: Etienne Carriere etienne.carriere@linaro.org

jforissier
jforissier reviewed Nov 30, 2022
View reviewed changes
Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

With the comments below addressed, LGTM.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Makefile Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@jenswi-linaro
Copy link
Contributor

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

eaaltonen
eaaltonen reviewed Dec 5, 2022
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
etienne-lms
etienne-lms commented Dec 5, 2022
View reviewed changes
Copy link
Contributor Author

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

updated with comments addressed, maybe to much: i've added some exptra space char but in the end, i don't think it's really expected.

Makefile Outdated Show resolved Hide resolved
@eaaltonen
Copy link
Contributor

Reviewed-by: Eero Aaltonen <eero.aaltonen@vaisala.com>
Tested-by as well, if you use it (built the 4 combinations and visually inspected the install tree).

@etienne-lms
Copy link
Contributor Author

etienne-lms commented Dec 7, 2022 •
edited

Thanks @eaaltonen for the review and testing.
I'll get rid of the added space char. CMake litterature does not seem to recommend or suggest them.

built the 4 combinations ...

You mean CFG_WERROR/WITH_TEEACL conbinaisions ?

@etienne-lms
Copy link
Contributor Author

I've squashed the fixed commit and removed the added space characters.
Review tags applied (I think still they apply with removed space chars).

@eaaltonen
Copy link
Contributor

You mean CFG_WERROR/WITH_TEEACL conbinaisions ?

I actually meant the combinations of echo {make,cmake},WITH_TEEACL={0,1}.

@jforissier
Copy link
Contributor

You mean CFG_WERROR/WITH_TEEACL conbinaisions ?

I actually meant the combinations of echo {make,cmake},WITH_TEEACL={0,1}.

I suggest adding a few lines to .github/workflows/ci.yml to make sure everything is tested

eaaltonen
eaaltonen reviewed Dec 8, 2022
View reviewed changes
CMakeLists.txt Outdated
@@ -37,7 +38,9 @@ add_subdirectory (libteec)
add_subdirectory (tee-supplicant)
add_subdirectory (public)
add_subdirectory (libckteec)
if(WITH_TEEACL)
find_package (PkgConfig REQUIRED)
pkg_check_modules(uuid REQUIRED IMPORTED_TARGET uuid)
add_subdirectory (libteeacl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just now realized that should probably also indent the block within the if(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in fixup commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 feel free to squash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks.

@etienne-lms
libteeacl: condition libteeacl with WITH_TEEACL
8e06044 
Build and embed libteeacl upon WITH_TEEACL=1 (default configuration).
This configuration switch allows one to build OP-TEE client without
dependencies on pkg-config and libuuid  when OP-TEE ACL for
PKCS11 is not needed:
 cmake -DWITH_TEEACL=0 ...
or
 make WITH_TEEACL=0 ...

With the comments below addressed, LGTM.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Eero Aaltonen <eero.aaltonen@vaisala.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
@etienne-lms
cmake: define boolean switch CFG_WERROR with option()
96767b6 
Replaces use of set() with option() for CFG_WERROR boolean switch.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Eero Aaltonen <eero.aaltonen@vaisala.com>
Tested-by: Eero Aaltonen <eero.aaltonen@vaisala.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
@etienne-lms
Copy link
Contributor Author

Addressed review comment. Tags are already applied.

@jforissier jforissier merged commit 1056b40 into OP-TEE:master Dec 8, 2022
@etienne-lms etienne-lms deleted the cfg-teeacl branch February 9, 2023 06:56
@etienne-lms etienne-lms mentioned this pull request Dec 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eaaltonen eaaltonen eaaltonen left review comments

@jforissier jforissier jforissier left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@etienne-lms @jenswi-linaro @eaaltonen @jforissier