Skip to content

If not targeting an architecture family don't run it's compiler #501

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

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

Conversation

ratmice
Copy link

@ratmice ratmice commented Apr 9, 2025

This is related to #377, when running

TOCK_TARGETS="cortex-m4" ./build_all.sh

One ends up getting a lot of messages if they don't have the riscv compiler installed,

make: riscv64-unknown-elf-gcc: No such file or directory

This appears to be because Configuration.mk runs the compiler to try and extract the compiler version.
This patch tries to only run the compiler/extract the version if the TOCK_TARGETS contains the string rv32.

I tested two cases:

  • With TOCK_TARGETS="cortex-m4", ensuring it doesn't produce the error
  • Without TOCK_TARGETS set, ensuring it still does because the default value contains the string.

@ratmice
Copy link
Author

ratmice commented Apr 9, 2025

May want to hold off on this, I still get the error running:
Edit: Nevermind, I think it's just TOCK_BOARD having been deprecated/removed.

$ make TOCK_BOARD=nrf52840dk

because TOCK_BOARD doesn't influence TOCK_TARGETS, so this error still happens sometimes.
Hmm, I think perhaps TOCK_BOARD doesn't actually do anything at all, at least ripgrep doesn't turn anything up.

Should it rather be removed from here:
https://github.com/tock/tock/blob/master/boards/nordic/nrf52dk/README.md

@ratmice ratmice force-pushed the dont_run_riscv_when_not_target branch from 61b2658 to c84283a Compare April 9, 2025 23:14
@ppannuto
Copy link
Member

You can check TOCK_ARCH_FAMILIES instead of parsing the targets again yourself, e.g. out of the box

TOCK_ARCH_FAMILIES=cortex-m rv32i

It'd be reasonable to add that to the config print from the verbose build to improve discoverability as part of this PR probably.


Yes, TOCK_BOARD should be removed from the documentation. I think the last vestiges of it were removed from here around a year ago as tockloader improved. It's also in imix documentation, which looks like it has more than should be purged, e.g., I don't think any of this paragraph is valid with modern tockloader.

@ratmice ratmice force-pushed the dont_run_riscv_when_not_target branch from c84283a to 59ea614 Compare April 10, 2025 05:41
@ratmice
Copy link
Author

ratmice commented Apr 10, 2025

Thanks, I used TOCK_ARCH_FAMILIES, and added it to the verbose build output like you asked.

Yeah, I'll work on a patch for the nordic stuff that I can figure out, but I was really not sure what to do about imix especially the parts about the application address, so I'll probably just leave that alone and make a note of that in an issue.

@@ -686,6 +688,7 @@ ifneq ($(V),)
$(info MAKEFLAGS=$(MAKEFLAGS))
$(info PACKAGE_NAME=$(PACKAGE_NAME))
$(info TOCK_ARCHS=$(TOCK_ARCHS))
$(info TOCK_ARCH_FAMILIES=$(TOCK_ARCH_FAMILIES))
Copy link
Author

Choose a reason for hiding this comment

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

There is apparently one more place to do this check in this configuration dump.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in ab7925a

@ratmice ratmice force-pushed the dont_run_riscv_when_not_target branch from 59ea614 to ab7925a Compare April 10, 2025 06:15
@ratmice
Copy link
Author

ratmice commented Apr 10, 2025

I figured I should also do the same for the cortex-m family.

@ratmice ratmice changed the title If not targeting riscv don't run it's compiler If not targeting an architecture family don't run it's compiler Apr 10, 2025
@ppannuto
Copy link
Member

This all looks good to me, thanks for the patches!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants