| Message ID | 20220315135646.3687446-1-arnout@mind.be |
|---|---|
| State | New |
| Headers | show |
| Series | sdk: support uppercase characters in MACHINE | expand |
On Tue, 2022-03-15 at 14:56 +0100, Arnout Vandecappelle (Essensium/Mind) wrote: > There are no real constraints on the values that MACHINE can take, it's > perfectly possible to define a machine with uppercase letters, and > building for that machine works fine. > > However, the (extensible) SDK targets include packages and packagegroups > that have MACHINE in their name, and uppercase letters are not allowed > in package names. > > Therefore, convert the machine name to lowercase when it's used as a > package name: for meta-environment, meta-environment-extsdk, > packagegroup-cross-canadian, packagegroup-go-cross-canadian and > packagegroup-rust-cross-canadian. Propagate this change to other places > where those packages are referred to. > > Alternatively, it would be possible to outright disallow uppercase > letters in MACHINE. However, this would affect any existing > configuration that currently works fine except for building the SDK. > > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > --- > I have only tested this with the base SDK, not with the extensible SDK. > I also haven't tested the changes to tclibc-baremetal, tclibc-newlib, > packagegroup-go-cross-canadian, packagegroup-rust-cross-canadian and > maintainers.inc. packagegroup-rust-cross-canadian doesn't even seem to > be used anywhere, so I guess this change would need to be propagated > somewhere externally. > > Ideally in addition to lowercasing, any other illegal package name > variables should also be removed or converted. It's less likely though > that other invalid characters would be used, since that will easily > break existing recipes. Also I couldn't find a conventient function to > make such a conversion. MACHINE is used in OVERRIDES and we only allow lower case overrides. The reasons for that are becoming more historical however the code still does require that and I'm not sure I see a pressing need to support mixed case or uppercase letters in MACHINE and complicate the code like this. Which version of the project allows MACHINE to have uppercase characters? Are you on a very old release? Cheers, Richard
Hi Arnout, On 3/15/22 14:56, Arnout Vandecappelle (Essensium/Mind) wrote: > There are no real constraints on the values that MACHINE can take, it's > perfectly possible to define a machine with uppercase letters, and > building for that machine works fine. > > However, the (extensible) SDK targets include packages and packagegroups > that have MACHINE in their name, and uppercase letters are not allowed > in package names. > > Therefore, convert the machine name to lowercase when it's used as a > package name: for meta-environment, meta-environment-extsdk, > packagegroup-cross-canadian, packagegroup-go-cross-canadian and > packagegroup-rust-cross-canadian. Propagate this change to other places > where those packages are referred to. > > Alternatively, it would be possible to outright disallow uppercase > letters in MACHINE. However, this would affect any existing > configuration that currently works fine except for building the SDK. > Overrides for machines starting with an uppercase letter have been broken and might still be (I was bitten by this on Thud). I don't know if this issue still applies after the migration to the newer override syntax though. e.g. FOO_Machine will not override FOO for Machine, it'll just create a new variable named FOO_Machine instead. Also, I seem to recall some weird issue with uppercase letter in the middle of package/recipe names. Bootlin discovered that issue in their training based on Rocko (?) a few years back because of people trying to use nInvaders as the recipe name instead of ninvaders. Pretty sure it was Alexandre who found the issue (don't remember if we ultimately found out what the issue was or just recommended not using uppercase letters at all). > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > --- > I have only tested this with the base SDK, not with the extensible SDK. > I also haven't tested the changes to tclibc-baremetal, tclibc-newlib, > packagegroup-go-cross-canadian, packagegroup-rust-cross-canadian and > maintainers.inc. packagegroup-rust-cross-canadian doesn't even seem to > be used anywhere, so I guess this change would need to be propagated > somewhere externally. > > Ideally in addition to lowercasing, any other illegal package name > variables should also be removed or converted. It's less likely though > that other invalid characters would be used, since that will easily > break existing recipes. Also I couldn't find a conventient function to > make such a conversion. I'm not sure it's worth the added complexity and confusion instead of the other way to do it which is to check for "harmful" characters in some critical strings and flat out fail the build if one is met. But I guess something needs to be done. https://docs.yoctoproject.org/bitbake/bitbake-user-manual/bitbake-user-manual-metadata.html#conditional-metadata is the only place where I could read something about the allowed characters. I guess we should explicit this in some other places, such as MACHINE in the ref-manual. Cheers, Quentin
On 15/03/2022 15:07, Richard Purdie wrote: > On Tue, 2022-03-15 at 14:56 +0100, Arnout Vandecappelle (Essensium/Mind) wrote: >> There are no real constraints on the values that MACHINE can take, it's >> perfectly possible to define a machine with uppercase letters, and >> building for that machine works fine. >> >> However, the (extensible) SDK targets include packages and packagegroups >> that have MACHINE in their name, and uppercase letters are not allowed >> in package names. >> >> Therefore, convert the machine name to lowercase when it's used as a >> package name: for meta-environment, meta-environment-extsdk, >> packagegroup-cross-canadian, packagegroup-go-cross-canadian and >> packagegroup-rust-cross-canadian. Propagate this change to other places >> where those packages are referred to. >> >> Alternatively, it would be possible to outright disallow uppercase >> letters in MACHINE. However, this would affect any existing >> configuration that currently works fine except for building the SDK. >> >> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> >> --- >> I have only tested this with the base SDK, not with the extensible SDK. >> I also haven't tested the changes to tclibc-baremetal, tclibc-newlib, >> packagegroup-go-cross-canadian, packagegroup-rust-cross-canadian and >> maintainers.inc. packagegroup-rust-cross-canadian doesn't even seem to >> be used anywhere, so I guess this change would need to be propagated >> somewhere externally. >> >> Ideally in addition to lowercasing, any other illegal package name >> variables should also be removed or converted. It's less likely though >> that other invalid characters would be used, since that will easily >> break existing recipes. Also I couldn't find a conventient function to >> make such a conversion. > > MACHINE is used in OVERRIDES and we only allow lower case overrides. The reasons > for that are becoming more historical however the code still does require that > and I'm not sure I see a pressing need to support mixed case or uppercase > letters in MACHINE and complicate the code like this. > > Which version of the project allows MACHINE to have uppercase characters? Are > you on a very old release? On dunfell. We have (received from someone else) two boards that have uppercase characters in MACHINE. And now I notice that the other board (for which I haven't tried generating the SDK yet) even has underscores in its name, which is also illegal in PN... I guess neither of these boards require overrides. I guess I better ask our supplier to change the name to lowercase and without underscores. Regards, Arnout
On 15/03/2022 15:24, Quentin Schulz wrote: > Hi Arnout, > > On 3/15/22 14:56, Arnout Vandecappelle (Essensium/Mind) wrote: >> There are no real constraints on the values that MACHINE can take, it's >> perfectly possible to define a machine with uppercase letters, and >> building for that machine works fine. >> >> However, the (extensible) SDK targets include packages and packagegroups >> that have MACHINE in their name, and uppercase letters are not allowed >> in package names. >> >> Therefore, convert the machine name to lowercase when it's used as a >> package name: for meta-environment, meta-environment-extsdk, >> packagegroup-cross-canadian, packagegroup-go-cross-canadian and >> packagegroup-rust-cross-canadian. Propagate this change to other places >> where those packages are referred to. >> >> Alternatively, it would be possible to outright disallow uppercase >> letters in MACHINE. However, this would affect any existing >> configuration that currently works fine except for building the SDK. >> > > Overrides for machines starting with an uppercase letter have been broken and > might still be (I was bitten by this on Thud). I don't know if this issue > still applies after the migration to the newer override syntax though. > > e.g. FOO_Machine will not override FOO for Machine, it'll just create a new > variable named FOO_Machine instead. > > Also, I seem to recall some weird issue with uppercase letter in the middle of > package/recipe names. Bootlin discovered that issue in their training based on > Rocko (?) a few years back because of people trying to use nInvaders as the > recipe name instead of ninvaders. Pretty sure it was Alexandre who found the > issue (don't remember if we ultimately found out what the issue was or just > recommended not using uppercase letters at all). > >> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> >> --- >> I have only tested this with the base SDK, not with the extensible SDK. >> I also haven't tested the changes to tclibc-baremetal, tclibc-newlib, >> packagegroup-go-cross-canadian, packagegroup-rust-cross-canadian and >> maintainers.inc. packagegroup-rust-cross-canadian doesn't even seem to >> be used anywhere, so I guess this change would need to be propagated >> somewhere externally. >> >> Ideally in addition to lowercasing, any other illegal package name >> variables should also be removed or converted. It's less likely though >> that other invalid characters would be used, since that will easily >> break existing recipes. Also I couldn't find a conventient function to >> make such a conversion. > > I'm not sure it's worth the added complexity and confusion instead of the > other way to do it which is to check for "harmful" characters in some critical > strings and flat out fail the build if one is met. But I guess something needs > to be done. > > https://docs.yoctoproject.org/bitbake/bitbake-user-manual/bitbake-user-manual-metadata.html#conditional-metadata > is the only place where I could read something about the allowed characters. I > guess we should explicit this in some other places, such as MACHINE in the > ref-manual. Ack, adding the list of allowed characters ([a-z0-9+-] I think - . is also allowed in PN but I'm not sure if it's OK for OVERRIDES) to the MACHINE documentation seems like an excellent idea. Even better would be a check (insane or other) that MACHINE is valid. Regards, Arnout
diff --git a/meta/classes/populate_sdk_base.bbclass b/meta/classes/populate_sdk_base.bbclass index 16f929bf59..d58c881cc3 100644 --- a/meta/classes/populate_sdk_base.bbclass +++ b/meta/classes/populate_sdk_base.bbclass @@ -43,7 +43,7 @@ B:task-populate-sdk = "${SDK_DIR}" SDKTARGETSYSROOT = "${SDKPATH}/sysroots/${REAL_MULTIMACH_TARGET_SYS}" -TOOLCHAIN_HOST_TASK ?= "nativesdk-packagegroup-sdk-host packagegroup-cross-canadian-${MACHINE}" +TOOLCHAIN_HOST_TASK ?= "nativesdk-packagegroup-sdk-host packagegroup-cross-canadian-${@d.getVar('MACHINE', True).lower()}" TOOLCHAIN_HOST_TASK_ATTEMPTONLY ?= "" TOOLCHAIN_TARGET_TASK ?= "${@multilib_pkg_extend(d, 'packagegroup-core-standalone-sdk-target')} target-sdk-provides-dummy" TOOLCHAIN_TARGET_TASK_ATTEMPTONLY ?= "" diff --git a/meta/classes/populate_sdk_ext.bbclass b/meta/classes/populate_sdk_ext.bbclass index e2019f9bbf..f2a2d9cde0 100644 --- a/meta/classes/populate_sdk_ext.bbclass +++ b/meta/classes/populate_sdk_ext.bbclass @@ -4,7 +4,7 @@ inherit populate_sdk_base # Used to override TOOLCHAIN_HOST_TASK in the eSDK case TOOLCHAIN_HOST_TASK_ESDK = " \ - meta-environment-extsdk-${MACHINE} \ + meta-environment-extsdk-${@d.getVar('MACHINE', True).lower()} \ " SDK_RELOCATE_AFTER_INSTALL:task-populate-sdk-ext = "0" diff --git a/meta/conf/distro/include/maintainers.inc b/meta/conf/distro/include/maintainers.inc index a8eceaadf4..6d1b3f4ced 100644 --- a/meta/conf/distro/include/maintainers.inc +++ b/meta/conf/distro/include/maintainers.inc @@ -495,8 +495,8 @@ RECIPE_MAINTAINER:pn-mesa = "Otavio Salvador <otavio.salvador@ossystems.com.br>" RECIPE_MAINTAINER:pn-mesa-demos = "Otavio Salvador <otavio.salvador@ossystems.com.br>" RECIPE_MAINTAINER:pn-mesa-gl = "Otavio Salvador <otavio.salvador@ossystems.com.br>" RECIPE_MAINTAINER:pn-meson = "Alexander Kanavin <alex.kanavin@gmail.com>" -RECIPE_MAINTAINER:pn-meta-environment-${MACHINE} = "Richard Purdie <richard.purdie@linuxfoundation.org>" -RECIPE_MAINTAINER:pn-meta-environment-extsdk-${MACHINE} = "Richard Purdie <richard.purdie@linuxfoundation.org>" +RECIPE_MAINTAINER:pn-meta-environment-${@d.getVar('MACHINE', True).lower()} = "Richard Purdie <richard.purdie@linuxfoundation.org>" +RECIPE_MAINTAINER:pn-meta-environment-extsdk-${@d.getVar('MACHINE', True).lower()} = "Richard Purdie <richard.purdie@linuxfoundation.org>" RECIPE_MAINTAINER:pn-meta-extsdk-toolchain = "Richard Purdie <richard.purdie@linuxfoundation.org>" RECIPE_MAINTAINER:pn-meta-go-toolchain = "Richard Purdie <richard.purdie@linuxfoundation.org>" RECIPE_MAINTAINER:pn-meta-ide-support = "Richard Purdie <richard.purdie@linuxfoundation.org>" diff --git a/meta/conf/distro/include/tclibc-baremetal.inc b/meta/conf/distro/include/tclibc-baremetal.inc index f3d27bbaae..2e216a4158 100644 --- a/meta/conf/distro/include/tclibc-baremetal.inc +++ b/meta/conf/distro/include/tclibc-baremetal.inc @@ -27,7 +27,7 @@ BASEDEPENDS:remove:class-target = "virtual/${TARGET_PREFIX}compilerlibs" TARGET_OS = "elf" TARGET_OS:arm = "eabi" -TOOLCHAIN_HOST_TASK ?= "packagegroup-cross-canadian-${MACHINE} nativesdk-qemu nativesdk-sdk-provides-dummy" +TOOLCHAIN_HOST_TASK ?= "packagegroup-cross-canadian-${@d.getVar('MACHINE', True).lower()} nativesdk-qemu nativesdk-sdk-provides-dummy" TOOLCHAIN_HOST_TASK_ATTEMPTONLY ?= "" TOOLCHAIN_TARGET_TASK ?= "libgcc-dev" TOOLCHAIN_NEED_CONFIGSITE_CACHE:remove = "virtual/${MLPREFIX}libc zlib ncurses" diff --git a/meta/conf/distro/include/tclibc-newlib.inc b/meta/conf/distro/include/tclibc-newlib.inc index 238b430e49..25c03f0aa4 100644 --- a/meta/conf/distro/include/tclibc-newlib.inc +++ b/meta/conf/distro/include/tclibc-newlib.inc @@ -38,7 +38,7 @@ BASE_DEFAULT_DEPS:append:class-target = " ${NEWLIB_EXTENDED}" TARGET_OS = "elf" TARGET_OS:arm = "eabi" -TOOLCHAIN_HOST_TASK ?= "packagegroup-cross-canadian-${MACHINE} nativesdk-qemu nativesdk-sdk-provides-dummy" +TOOLCHAIN_HOST_TASK ?= "packagegroup-cross-canadian-${@d.getVar('MACHINE', True).lower()} nativesdk-qemu nativesdk-sdk-provides-dummy" TOOLCHAIN_TARGET_TASK ?= "${LIBC_DEPENDENCIES}" TOOLCHAIN_NEED_CONFIGSITE_CACHE:remove = "zlib ncurses" diff --git a/meta/recipes-core/meta/meta-environment-extsdk.bb b/meta/recipes-core/meta/meta-environment-extsdk.bb index 706312b0d6..15a86c613c 100644 --- a/meta/recipes-core/meta/meta-environment-extsdk.bb +++ b/meta/recipes-core/meta/meta-environment-extsdk.bb @@ -2,7 +2,7 @@ require meta-environment.bb -PN = "meta-environment-extsdk-${MACHINE}" +PN = "meta-environment-extsdk-${@d.getVar('MACHINE', True).lower()}" create_sdk_files:append() { local sysroot=${SDKPATH}/tmp/${@os.path.relpath(d.getVar('STAGING_DIR'), d.getVar('TMPDIR'))}/${MACHINE} diff --git a/meta/recipes-core/meta/meta-environment.bb b/meta/recipes-core/meta/meta-environment.bb index 7118fb2aef..146f90e7df 100644 --- a/meta/recipes-core/meta/meta-environment.bb +++ b/meta/recipes-core/meta/meta-environment.bb @@ -69,7 +69,7 @@ do_install() { install -m 0644 -t ${D}/${SDKPATH} ${SDK_OUTPUT}/${SDKPATH}/* } -PN = "meta-environment-${MACHINE}" +PN = "meta-environment-${@d.getVar('MACHINE', True).lower()}" PACKAGES = "${PN}" FILES:${PN}= " \ ${SDKPATH}/* \ diff --git a/meta/recipes-core/meta/meta-go-toolchain.bb b/meta/recipes-core/meta/meta-go-toolchain.bb index c24518efe3..4d405f9b94 100644 --- a/meta/recipes-core/meta/meta-go-toolchain.bb +++ b/meta/recipes-core/meta/meta-go-toolchain.bb @@ -4,7 +4,7 @@ LICENSE = "MIT" inherit populate_sdk TOOLCHAIN_HOST_TASK:append = " \ - packagegroup-go-cross-canadian-${MACHINE} \ + packagegroup-go-cross-canadian-${@d.getVar('MACHINE', True).lower()} \ " TOOLCHAIN_TARGET_TASK:append = " \ diff --git a/meta/recipes-core/packagegroups/packagegroup-cross-canadian.bb b/meta/recipes-core/packagegroups/packagegroup-cross-canadian.bb index 49c075eb11..80cc202233 100644 --- a/meta/recipes-core/packagegroups/packagegroup-cross-canadian.bb +++ b/meta/recipes-core/packagegroups/packagegroup-cross-canadian.bb @@ -1,5 +1,5 @@ SUMMARY = "Host SDK package for cross canadian toolchain" -PN = "packagegroup-cross-canadian-${MACHINE}" +PN = "packagegroup-cross-canadian-${@d.getVar('MACHINE', True).lower()}" inherit cross-canadian packagegroup @@ -14,7 +14,7 @@ RDEPENDS:${PN} = "\ ${@all_multilib_tune_values(d, 'BINUTILS')} \ ${@all_multilib_tune_values(d, 'GCC')} \ ${@all_multilib_tune_values(d, 'GDB')} \ - meta-environment-${MACHINE} \ + meta-environment-${@d.getVar('MACHINE', True).lower()} \ " # When TUNE_ARCH changes but MACHINE does not (for example when a machine definition is updated), diff --git a/meta/recipes-core/packagegroups/packagegroup-go-cross-canadian.bb b/meta/recipes-core/packagegroups/packagegroup-go-cross-canadian.bb index d0596efe7a..6b10e816ba 100644 --- a/meta/recipes-core/packagegroups/packagegroup-go-cross-canadian.bb +++ b/meta/recipes-core/packagegroups/packagegroup-go-cross-canadian.bb @@ -1,5 +1,5 @@ SUMMARY = "Host SDK package for Go cross canadian toolchain" -PN = "packagegroup-go-cross-canadian-${MACHINE}" +PN = "packagegroup-go-cross-canadian-${@d.getVar('MACHINE', True).lower()}" inherit cross-canadian packagegroup diff --git a/meta/recipes-core/packagegroups/packagegroup-rust-cross-canadian.bb b/meta/recipes-core/packagegroups/packagegroup-rust-cross-canadian.bb index 0d4f5ec9ef..4a347cba34 100644 --- a/meta/recipes-core/packagegroups/packagegroup-rust-cross-canadian.bb +++ b/meta/recipes-core/packagegroups/packagegroup-rust-cross-canadian.bb @@ -1,5 +1,5 @@ SUMMARY = "Host SDK package for Rust cross canadian toolchain" -PN = "packagegroup-rust-cross-canadian-${MACHINE}" +PN = "packagegroup-rust-cross-canadian-${@d.getVar('MACHINE', True).lower()}" inherit cross-canadian packagegroup