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

Set linker script from build.rs file #88

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

jwnrt
Copy link
Contributor

@jwnrt jwnrt commented Jan 16, 2024

This has two advantages:

  1. It adds a cargo:rerun-if-changed command to ensure we rebuild crates if the linker script changes.
  2. The path to the linker script does not depend on the directory that Cargo is executed from, so cargo build can be done from anywhere.

@jwnrt jwnrt requested a review from engdoreis January 16, 2024 15:15
Copy link
Contributor

@engdoreis engdoreis left a comment

Choose a reason for hiding this comment

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

Thanks @jwnrt.
How can we make it clear where the linker script is added? Maybe we could document on each Cargo.toml or print a println! on build.rs?

This has two advantages:

1. It adds a `cargo:rerun-if-changed` command to ensure we rebuild
   crates if the linker script changes.
2. The path to the linker script does not depend on the directory that
   Cargo is executed from, so `cargo build` can be done from anywhere.

Signed-off-by: James Wainwright <[email protected]>
@jwnrt
Copy link
Contributor Author

jwnrt commented Jan 18, 2024

I have added a comment to the demo Cargo.toml files because println! in build.rs is hidden unless there's an error

@nbdd0121
Copy link
Contributor

The path to the linker script does not depend on the directory that Cargo is executed from, so cargo build can be done from anywhere.

I think this one is not true, but rerun-if-changed is still a plus.

@jwnrt
Copy link
Contributor Author

jwnrt commented Jan 18, 2024

I think this one is not true, but rerun-if-changed is still a plus.

You're right - I was mistaken about the error message you get with cargo build. It was actually saying that there wasn't enough space to link correctly, not that the link was badly defined.

I've added an extra commit to enable optimizations for debug builds (level 1 for the crate and 2 for dependencies) so that cargo build just works.

@jwnrt jwnrt requested a review from engdoreis January 18, 2024 11:41
@engdoreis
Copy link
Contributor

It was actually saying that there wasn't enough space to link correctly, not that the link was badly defined.

@alees24 Have increased the total SRAM to 128 kB, When it gets merged we won't have this issue for a while.

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. I have not tested it so it would be good to have someone with more Rust knowledge to approve it as well.

@jwnrt jwnrt merged commit 82a3d48 into lowRISC:main Feb 1, 2024
2 checks passed
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.

4 participants