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

Jump to bottom

Use fixed architecture names in toolchain wrapper #43

Merged
merged 1 commit into from
Mar 10, 2025
Merged

Use fixed architecture names in toolchain wrapper #43

merged 1 commit into from
Mar 10, 2025

Conversation

troibe
Copy link
Contributor

@troibe troibe commented Mar 7, 2025

uname -m seems to not be available in some sandboxed environments. This error only occurred after an hour of CI tests on the main opentitan repo.

@troibe
Use fixed architecture names in toolchain wrapper
d3bc95f 
`uname -m` seems to not be available in some sandboxed environments. This error only occurred after an hour of CI tests on the main opentitan repo.
@troibe troibe requested review from jwnrt, nbdd0121 and pamaury March 7, 2025 11:40
@troibe
Copy link
Contributor Author

troibe commented Mar 7, 2025 •
edited
Loading

Follow-up to #42
Full opentitan CI run can be found here: lowRISC/opentitan#26476

pamaury
pamaury approved these changes Mar 7, 2025
View reviewed changes
Copy link
Collaborator

@pamaury pamaury left a comment

Choose a reason for hiding this comment

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

I will approve to unblock but I just want to point out that an alternative approach would be to use bazel to create those files with a template. This would scale better.

@troibe
Copy link
Contributor Author

troibe commented Mar 7, 2025

Thanks for the approval and also the suggestion.
I just had a quick look into the template generation
but the usage of symlinks here seems to complicate the template usage.
So if this fixed structure is fine for now it's probably easier that way...

@troibe troibe mentioned this pull request Mar 7, 2025
Merged
jwnrt
jwnrt approved these changes Mar 7, 2025
View reviewed changes
Copy link
Contributor

@jwnrt jwnrt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm okay with this duplication but @pamaury is right that it won't scale.

I wonder if @cfrantz would be able to yes/no this?

cfrantz
cfrantz reviewed Mar 7, 2025
View reviewed changes
toolchains/lowrisc_rv32imcb/wrappers_aarch64/driver.sh
@@ -4,7 +4,7 @@
# SPDX-License-Identifier: Apache-2.0

PROG=${0##*/}
TOOLCHAIN="lowrisc_rv32imcb_$(uname -m)_files"
TOOLCHAIN="lowrisc_rv32imcb_aarch64_files"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be nice if there was a way for bazel to pass this env var in as part of the tool configuration. I did a bit of looking, but I didn't see anything obvious.

I'm also a bit surprised that uname is not available.

Copy link
Contributor Author

@troibe troibe Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yep that surprised me too. There were only a few specific sandbox environments where it failed to find uname.

  = note: external/crt/toolchains/lowrisc_rv32imcb/wrappers/clang: line 7: uname: command not found
          external/crt/toolchains/lowrisc_rv32imcb/wrappers/clang: line 13: /home/runner/.cache/bazel/_bazel_runner/009b5905a925a435539a99f3bc28ac3e/sandbox/processwrapper-sandbox/2923/execroot/lowrisc_opentitan/external/lowrisc_rv32imcb__files/bin/riscv32-unknown-elf-clang: No such file or directory

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible to pass environment variables to tools as part of the toolchain configuration but unfortunately it is a rather non-trivial change because it requires to use env_set and env_entry which we have no reified in the crt (I do have a patch for that but it's quite useless at this point since we do not use the crt for upstream). If you really do need it I can make a PR for that.

Copy link

Choose a reason for hiding this comment

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

I wonder if the issue is that PATH is hidden by Bazel so non-builtin commands will not work at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to quickly follow up on this. @jwnrt @cfrantz
I have to admit I got a bit confused about the feedback.
Is there something straightforward to improve here or is this ok to merge & release?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think let's merge and release this and we can investigate the uname issue later if it becomes an issue

@jwnrt jwnrt merged commit f5cbfb1 into lowRISC:main Mar 10, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pamaury pamaury pamaury approved these changes

@cfrantz cfrantz cfrantz left review comments

@jwnrt jwnrt jwnrt approved these changes

@nbdd0121 nbdd0121 Awaiting requested review from nbdd0121

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@troibe @pamaury @cfrantz @nbdd0121 @jwnrt