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

Track the missing dependencies on SPDX documents #1109

Merged
merged 15 commits into from
May 9, 2023

Conversation

quaresmajose
Copy link
Member

@quaresmajose quaresmajose commented Apr 18, 2023

This handler will generate an ERROR when there are 'do_deploy' dependencies without setting the DEPENDS.
One of the side effect of this issues is SPDX documents not including the correct dependencies.

/
| ERROR: lmp-boot-firmware-0-r0 do_install: Task 'do_install' depends on 'virtual/bootloader:do_deploy' but 'virtual/bootloader' is not in DEPENDS
| ERROR: lmp-boot-firmware-0-r0 do_install: Task 'do_install' depends on 'virtual/trusted-firmware-a:do_deploy' but 'virtual/trusted-firmware-a' is not in DEPENDS
\

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package as well as in the final SPDX of the combined image.

The following example is for the lmp-base-console-image on stm32mp15-disco machine:

/
| grep -E "u-boot|tf-a" deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "documentNamespace": "http://spdx.org/spdxdoc/recipe-tf-a-fio-546ca65d-3ceb-5456-93cd-e8acffa32a17",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "filename": "recipe-tf-a-fio.spdx.json",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "documentNamespace": "http://spdx.org/spdxdoc/recipe-tf-a-tools-native-52687ea6-9f82-5a2d-a8ab-f4e90f31e6fd",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "filename": "recipe-tf-a-tools-native.spdx.json",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "documentNamespace": "http://spdx.org/spdxdoc/recipe-u-boot-fio-cfb5b3c2-fc87-5d33-a223-5b46ae9bb0f5",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "filename": "recipe-u-boot-fio.spdx.json",
\

@quaresmajose quaresmajose requested a review from a team April 18, 2023 15:16
@ricardosalveti
Copy link
Member

Please extend the commit messages to avoid patches with just the subject line (add more details as part of the commit body message).

@quaresmajose
Copy link
Member Author

I reduce this PR only with the fix and move the refactor to a new one.

@MrCry0
Copy link
Contributor

MrCry0 commented Apr 18, 2023

in the commit msg: s/combinad/combined/ ?

@quaresmajose quaresmajose force-pushed the lmp-boot-firmware branch 2 times, most recently from 5ef29b2 to 9576f23 Compare April 18, 2023 18:44
@quaresmajose
Copy link
Member Author

in the commit msg: s/combinad/combined/ ?

fixed! thanks

Copy link
Contributor

@MrCry0 MrCry0 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@angolini angolini left a comment

Choose a reason for hiding this comment

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

LGTM

python () {
# we need to set the DEPENDS as well to produce valid SPDX documents
for tasks in d.getVarFlag('do_install', 'depends').split():
d.appendVar('DEPENDS', " %s" % tasks.split(":")[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting DEPENDS usually goes on the top of the file. Is there any reason it must be at the bottom in this case?

Copy link
Member Author

@quaresmajose quaresmajose Apr 19, 2023

Choose a reason for hiding this comment

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

Nothing special, only because it is using a non common anonymous python.

@quaresmajose quaresmajose force-pushed the lmp-boot-firmware branch 2 times, most recently from 0abef67 to ee4404e Compare April 19, 2023 10:42
@quaresmajose quaresmajose marked this pull request as draft April 19, 2023 22:50
@quaresmajose quaresmajose force-pushed the lmp-boot-firmware branch 3 times, most recently from 83a8b45 to c1eb5f5 Compare April 21, 2023 11:33
@quaresmajose quaresmajose force-pushed the lmp-boot-firmware branch 2 times, most recently from 91f39ab to 48dd890 Compare April 21, 2023 15:05
@quaresmajose quaresmajose marked this pull request as ready for review April 21, 2023 15:08
@quaresmajose
Copy link
Member Author

validated on foundriesio/lmp-manifest#318

@quaresmajose quaresmajose changed the title Lmp boot firmware: track the missing dependencies on SPDX combined image Track the missing dependencies on SPDX documents Apr 21, 2023
for depend in depends:
recipe, task = depend.split(':')
if task == 'do_deploy' and recipe not in d.getVar('DEPENDS'):
bb.error("Task '%s' depends on '%s' but '%s' is not in DEPENDS" % (taskname, depend, recipe))
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better for us to warn by default instead of calling an error here, as this might have side effects in customer related recipes and cause certain builds to fail.

While we do want to have SPDX in a correct form, we shouldn't just yet enforce this on every recipe (we can have as a warn first and on another release move to error, for example).

Copy link
Member Author

Choose a reason for hiding this comment

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

I put a new commit 25a255e wih this that is simpler to reference later.

@quaresmajose quaresmajose force-pushed the lmp-boot-firmware branch 2 times, most recently from 0783ed6 to 0ba3026 Compare April 25, 2023 09:25
…n depends

This handler will generate an ERROR when there are 'do_deploy' dependencies without setting the DEPENDS.
One of the side effect of this issues is SPDX documents not including the correct dependencies.

/
| ERROR: lmp-boot-firmware-0-r0 do_install: Task 'do_install' depends on 'virtual/bootloader:do_deploy' but 'virtual/bootloader' is not in DEPENDS
| ERROR: lmp-boot-firmware-0-r0 do_install: Task 'do_install' depends on 'virtual/trusted-firmware-a:do_deploy' but 'virtual/trusted-firmware-a' is not in DEPENDS
\

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
Setting DEPENDS create dependency loops so skip the check

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
Setting DEPENDS create dependency loops so skip the check

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
… task[depends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

The following example is for the lmp-base-console-image on stm32mp15-disco machine:
/
| grep -E "u-boot|tf-a" deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "documentNamespace": "http://spdx.org/spdxdoc/recipe-tf-a-fio-546ca65d-3ceb-5456-93cd-e8acffa32a17",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "filename": "recipe-tf-a-fio.spdx.json",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "documentNamespace": "http://spdx.org/spdxdoc/recipe-tf-a-tools-native-52687ea6-9f82-5a2d-a8ab-f4e90f31e6fd",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "filename": "recipe-tf-a-tools-native.spdx.json",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "documentNamespace": "http://spdx.org/spdxdoc/recipe-u-boot-fio-cfb5b3c2-fc87-5d33-a223-5b46ae9bb0f5",
| deploy/images/stm32mp15-disco/lmp-base-console-image-stm32mp15-disco.spdx.index.json:      "filename": "recipe-u-boot-fio.spdx.json",
\

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
…pends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
…nce on task[depends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
…sk[depends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
…k[depends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
… on task[depends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
…nds]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
… on task[depends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
…epends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
…o_deploy' presence on task[depends]

Explicit adds the dependencies of the 'do_deploy' in the task[depends].
With this patch the missed dependencies are now in the SPDX documents of the package
as well as in the final SPDX of the combined image.

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
…f an error

It is better for us to warn by default instead of calling an error here,
as this might have side effects in customer related recipes and cause certain builds to fail.

While we do want to have SPDX in a correct form, we shouldn't just yet enforce this on every recipe
(we can have as a warn first and on another release move to error, for example).

Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io>
Copy link
Member

@ricardosalveti ricardosalveti left a comment

Choose a reason for hiding this comment

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

LGTM

@ricardosalveti ricardosalveti merged commit 6e74f8c into foundriesio:main May 9, 2023
@quaresmajose quaresmajose deleted the lmp-boot-firmware branch May 9, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants