-
Notifications
You must be signed in to change notification settings - Fork 26
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
build-toolchain.sh: add D and OpenMP checks #61
Conversation
Add checks for BR2_TOOLCHAIN_BUILDROOT_DLANG and BR2_GCC_ENABLE_OPENMP. Correctly flagging these options allows external consumers to properly configure options that may depend on them[0]. [0] https://bugs.buildroot.org/show_bug.cgi?id=15634#c3 Signed-off-by: Vincent Fazio <vfazio@gmail.com>
|
I have done a test build against this commit and see that the flags are toggled correctly: |
|
@RomainNaour @tpetazzoni Just wanted to ping you two on this. I wasn't sure if i should submit the PR here or in https://gitlab.com/buildroot.org/toolchains-builder |
| else | ||
| echo "# BR2_TOOLCHAIN_EXTERNAL_DLANG is not set" >> ${fragment_file} | ||
| fi | ||
| if grep -q "BR2_GCC_ENABLE_OPENMP=y" ${configfile}; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be BR2_TOOLCHAIN_HAS_OPENMP ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
if grep -q "BR2_TOOLCHAIN_BUILDROOT_FORTRAN=y" ${configfile}; then
should be
if grep -q "BR2_TOOLCHAIN_HAS_FORTRAN=y" ${configfile}; then
It seems this script is not really accurate...
Should we use BR2_TOOLCHAIN_BUILDROOT_xxx or BR2_TOOLCHAIN_HAS_xxx ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to use BR2_TOOLCHAIN_BUILDROOT_*
we're looking for the features the buildroot tool chain is being built with so that we can then flag the EXTERNAL flags when it's used by subsequent BR project as an external toolchain. The BR2_TOOLCHAIN_BUILDROOT_xxx and BR2_TOOLCHAIN_EXTERNAL_xxx (depending on the BR config) drive the unified BR2_TOOLCHAIN_HAS_xxx option for packages to check to see if a feature is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right but my comment is about how the script is written, the generated config fragment should be the same with either BR2_TOOLCHAIN_BUILDROOT_xxx or BR2_TOOLCHAIN_HAS_xxx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: BR2_TOOLCHAIN_EXTERNAL_DLANG will no longer be a problem since gcc 12 requires a D compiler on the host. Currently Buildroot doesn't check for DLANG on the host, so we can't enable the D backend on gcc 12.
See: https://gitlab.com/buildroot.org/buildroot/-/commit/6bd0cc0cb8a9a3016ca4970a21ebbf7f497dfe96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever we do needs to be reflected in https://gitlab.com/vfazio/buildroot/-/blob/248bdb63544ae4442cfbd20e32615bf12b3d8c9b/support/scripts/gen-bootlin-toolchains#L381
| @@ -164,6 +164,16 @@ function make_br_fragment { | |||
| else | |||
| echo "# BR2_TOOLCHAIN_EXTERNAL_FORTRAN is not set" >> ${fragment_file} | |||
| fi | |||
| if grep -q "BR2_TOOLCHAIN_BUILDROOT_DLANG=y" ${configfile}; then | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BR2_TOOLCHAIN_HAS_DLANG ?
No, the gitlab repo is where the builds are done. PR are handled from this github repo. |
|
Thanks, applied! |
Add checks for BR2_TOOLCHAIN_BUILDROOT_DLANG and BR2_GCC_ENABLE_OPENMP.
Correctly flagging these options allows external consumers to properly configure options that may depend on them[0].
[0] https://bugs.buildroot.org/show_bug.cgi?id=15634#c3
closes #60