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.

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

Small change for RISCV compilation #701

Closed
wants to merge 1 commit into from

Conversation

puma314
Copy link

@puma314 puma314 commented Jun 11, 2024

No description provided.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Please provide more details. Why is the change needed, what problem does it solve and why you picked this particular solution?


// if unset, try the default and fail eventually
DEFAULT_RISCV_GNU_TOOLCHAIN.into()
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's appropriate to force every RISCV compilation to use the GNU toolchains - are there not other toolchains?

base_config
.compiler("clang")
.no_default_flags(true)
.flag(&format!("--sysroot={riscv_gnu_toolchain_path}/riscv32-unknown-elf"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC this is beyond our MSRV.

@apoelstra
Copy link
Member

Agreed, I'd like a lot more details and a CI job that compiles with RISCV.

If we currently can't compile with riscv and now we can, that's cool, and I'd be willing to compromise on MSRV or other forms of flexibility to get that. But from this PR I can't tell what's going on.

@puma314
Copy link
Author

puma314 commented Jun 11, 2024

Sorry I accidentally opened this PR against the upstream vs. my own fork. Will close and then re-open with more details later.

@puma314 puma314 closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants