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

image_types_qcom.bbclass: Allow QCOMFLASH_DIR to be overridden #755

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

image_types_qcom.bbclass: Allow QCOMFLASH_DIR to be overridden #755

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion classes-recipe/image_types_qcom.bbclass
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ QCOM_DTB_FILE ?= "dtb.bin"
QCOM_ROOTFS_FILE ?= "rootfs.img"
IMAGE_QCOMFLASH_FS_TYPE ??= "ext4"

QCOMFLASH_DIR = "${IMGDEPLOYDIR}/${IMAGE_NAME}.qcomflash"
QCOMFLASH_DIR ?= "${IMGDEPLOYDIR}/${IMAGE_NAME}.qcomflash"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What for? Please provide an example rather than a vague "users may need to".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use here: qualcomm-linux/meta-qcom-distro@d045d04

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the point? You can still use the .qcomflash subdir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is wrong if qcomflash subdir is not used? This is not about what is correct and what is not. When a variable is defined, there is a general expectation that it can be overridden as needed. Making a hard assignment is not correct. If the intention is not to allow modifying the path, then it should not have been a variable and should have been hard-coded instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The variable can be defined in order to make reading the code easier, like the const in other languages. Some variables differ from configuration to the configuration, there is a need to override them. Others are common and as such there is no need to override them. If you think that the existing value is not optimal, please change it instead of overriding it just for the distro: it makes user experience much more difficult if the deploy subdirs start to depend on the $DISTRO.

Copy link
Contributor Author

@quic-vkraleti quic-vkraleti Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In house tools used by Qualcomm don't understand this directory organization. They are coded to work with the paths used in Kirkstone & Scarthgap branches of meta-qcom-hwe. (i.e. builddir/tmpdir/deploy/images/machine/imgName)

I am not comfortable discussing beyond this in a public forum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, either those tools need to be fixed, or you can change a default value here. Both options are fine with me.

P.S. This is a public layer now, so such discussions are to be public.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should have a different behavior for this in meta-qcom-distro, we should have a default and use the same on both, otherwise this will cause confusion to our users (e.g. documentation would be different when using just the BSP layer).

Can we evaluate changing the internal tools? We can discuss the internal tooling needs via email.

Copy link
Contributor

Choose a reason for hiding this comment

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

[...]If you think that the existing value is not optimal, please change it instead of overriding it just for the distro: it makes user experience much more difficult if the deploy subdirs start to depend on the $DISTRO.

Tangentially related to this: in Angstrom we moved DEPLOY_DIR out of TMPDIR to the top level to make it easier to find, since it contains the things people actually consume. So the DISTRO cleaning up how a BSP behaves is not unprecedented, but it does need careful consideration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, Angstrom moved the whole DEPLOY_DIR.

IMAGE_CMD:qcomflash = "create_qcomflash_pkg"
do_image_qcomflash[dirs] = "${QCOMFLASH_DIR}"
do_image_qcomflash[cleandirs] = "${QCOMFLASH_DIR}"
Expand Down
Loading