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

samples: tfm_integration: psa_crypto: Disable sample (prevent fetching online content) #76093

Merged
merged 1 commit into from
Jul 27, 2024

Conversation

nordicjm
Copy link
Collaborator

@nordicjm nordicjm commented Jul 19, 2024

Prevents cmake (and tfm) from fetching online content in CI. Added because yet again tfm is fetching online content for its build for the 4th time or so and we need to catch it in CI before it gets merged. Thanks to @JordanYates for discovering issue

Disables running this sample as doing so requires qcbor, which
is not apache licensed

fabiobaltieri
fabiobaltieri previously approved these changes Jul 19, 2024
@fabiobaltieri fabiobaltieri requested a review from a team July 19, 2024 09:31
@fabiobaltieri
Copy link
Member

fabiobaltieri commented Jul 19, 2024

Fixes #76090?

@nordicjm
Copy link
Collaborator Author

nordicjm commented Jul 19, 2024

Fixes #76090?

Does not fix, no, but prevents issues like that occurring again, that module still needs to be fixed by bringing in cmsis to tfm locally. The PR here rightfully fails, see https://github.com/zephyrproject-rtos/zephyr/actions/runs/10005520276/job/27656611035?pr=76093 which is what should be happening if a future update is brought in that does the same thing

@fabiobaltieri
Copy link
Member

Ok thanks for clarifying, should this be enabled in the main CMakeFile instead? I imagine not because it would prevent a user from using this cmake feature.

@nordicjm
Copy link
Collaborator Author

Ok thanks for clarifying, should this be enabled in the main CMakeFile instead? I imagine not because it would prevent a user from using this cmake feature.

It is enabled there in CI with the twister -x option but only in CI, and yes for that reason that end users might want to use the feature. The annoying thing about cmake is that seemingly this option cannot be set in the environment or propagated to additional invocations of cmake, you can only set it on a single instance, not great really.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jul 19, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@tomi-font
Copy link
Collaborator

Thanks for adding this. I opened a fix PR: #76094.
I am kind of wondering how this works. In the CI logs I see that the git apply command fails, which is something that happens after the git clone in TF-M. But no logs pertaining to the git clone. Does it just fail silently?

@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Jul 19, 2024
@nordicjm
Copy link
Collaborator Author

Thanks for adding this. I opened a fix PR: #76094. I am kind of wondering how this works. In the CI logs I see that the git apply command fails, which is something that happens after the git clone in TF-M. But no logs pertaining to the git clone. Does it just fail silently?

What this does is completely bypass the fetch steps, it's meant to be used with a cache that has already downloaded it, so it assumes that the data is present and then goes to the patching stage which then fails because the files do not exist, so it's not so much a case of it fails silently but just skips, if there was no file patching done then it would fail at the case of trying to build with the files instead

@JordanYates
Copy link
Collaborator

JordanYates commented Jul 19, 2024

Based on the documentation, the option explicitly shouldn't be used for this purpose:
https://cmake.org/cmake/help/latest/module/FetchContent.html#variable:FETCHCONTENT_FULLY_DISCONNECTED

Note
The FETCHCONTENT_FULLY_DISCONNECTED variable is not an appropriate way to prevent any network access on the first run in a build directory. Doing so can break projects, lead to misleading error messages, and hide subtle population failures. This variable is specifically intended to only be turned on after the first time CMake has been run. If you want to prevent network access even on the first run, use a dependency provider and populate the dependency from local content instead.

@nordicjm
Copy link
Collaborator Author

Based on the documentation, the option explicitly shouldn't be used for this purpose: https://cmake.org/cmake/help/latest/module/FetchContent.html#variable:FETCHCONTENT_FULLY_DISCONNECTED

Note
The FETCHCONTENT_FULLY_DISCONNECTED variable is not an appropriate way to prevent any network access on the first run in a build directory. Doing so can break projects, lead to misleading error messages, and hide subtle population failures. This variable is specifically intended to only be turned on after the first time CMake has been run. If you want to prevent network access even on the first run, use a dependency provider and populate the dependency from local content instead.

That is stating that it shouldn't be used because you should fetch the content first, which is exactly what you should do if you are expecting to use the fetch functionality. It's fine to use here because we do not want any fetching of content, all the files to build should already be present, fetching should only be used with upstream TF-M, not zephyr

@tomi-font
Copy link
Collaborator

Now samples/tfm_integration/psa_crypto/sample.psa_crypto fails because it has

# Enable the initial attestation
CONFIG_TFM_PARTITION_INITIAL_ATTESTATION=y
CONFIG_TFM_QCBOR_PATH="DOWNLOAD"

that results in TF-M attempting to download QCBOR.

However QCBOR is complicated because its license is not compatible with Zephyr, so cannot be included. Some background:

What shall we do with this test then? Disable it?

@fabiobaltieri
Copy link
Member

Module PR: zephyrproject-rtos/trusted-firmware-m#109 (also needed by #76094)

Disables running this sample as doing so requires qcbor, which
is not apache licensed

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm nordicjm force-pushed the preventfetchonline branch from 0f1693f to f39c9ee Compare July 25, 2024 07:22
@zephyrbot zephyrbot removed manifest manifest-trusted-firmware-m DNM This PR should not be merged (Do Not Merge) labels Jul 25, 2024
@nordicjm
Copy link
Collaborator Author

I have had to remove the commits that prevent online fetching as cmake have taken it amongst themselves to deprecate how it is being used and will silently chuck the feature in the future, not impressed. However the tests passed which means no content is being fetched, so I've left a commit here disabling the test that does still fetch an external module (that isn't apache licensed)

@nordicjm nordicjm added this to the v3.7.0 milestone Jul 25, 2024
@nordicjm nordicjm changed the title tfm and ci: Prevent fetching online content samples: tfm_integration: psa_crypto: Disable sample (prevent fetching online content) Jul 25, 2024
@nordicjm nordicjm assigned tomi-font and unassigned d3zd3z Jul 25, 2024
@nordicjm nordicjm requested a review from aescolar July 25, 2024 12:41
@aescolar aescolar added the backport v3.7-branch Request backport to the v3.7-branch label Jul 26, 2024
@aescolar aescolar removed this from the v3.7.0 milestone Jul 26, 2024
@nashif nashif merged commit db5a5fa into zephyrproject-rtos:main Jul 27, 2024
20 of 23 checks passed
@glarsennordic
Copy link
Contributor

@nordicjm Hi -- I am trying to understand the ramifications of this. I'm working on unit tests for some new features that rely on TFM. It seems to me that I will have to mark these new tests as disabled as well, for the same reason?

Also, with #76094 merged, is this still an issue? Can these tests be re-enabled?

@nordicjm
Copy link
Collaborator Author

@nordicjm Hi -- I am trying to understand the ramifications of this. I'm working on unit tests for some new features that rely on TFM. It seems to me that I will have to mark these new tests as disabled as well, for the same reason?

Also, with #76094 merged, is this still an issue? Can these tests be re-enabled?

If it no longer pulls down from online they can be re-enabled

@tomi-font
Copy link
Collaborator

#76094 was a separate issue than this.

This sample was disabled in CI because it pulls code during build. It's because QCBOR has a license that is incompatible with Zephyr (so Zephyr doesn't ship it). It's needed for CONFIG_TFM_PARTITION_INITIAL_ATTESTATION, which is enabled by this sample. So to re-enable this sample would require either disabling CONFIG_TFM_PARTITION_INITIAL_ATTESTATION, or fixing the licensing issues with QCBOR.
For more on this see Kconfig.tfm, CMakeLists.txt, and my previous message with linked issues.

Anyway, this doesn't prevent other TF-M-related tests/samples from being added. They don't even need to be disabled as long as they don't make use of QCBOR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Continuous Integration area: Samples Samples area: TF-M ARM Trusted Firmware-M (TF-M) backport v3.7-branch Request backport to the v3.7-branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.