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

Set linker script from build.rs file #88

Merged
merged 2 commits into from
Feb 1, 2024
Merged

Set linker script from build.rs file #88

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
engdoreis
engdoreis approved these changes Jan 18, 2024
View reviewed changes
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?

@jwnrt
Set linker script from build.rs file
0acffd2 
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 jwnrt force-pushed the cargo-build-linker-script branch from b9ccd89 to 0acffd2 Compare January 18, 2024 10:36
@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.

marnovandermaas
marnovandermaas approved these changes Jan 25, 2024
View reviewed changes
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
Reviewers

@marnovandermaas marnovandermaas marnovandermaas approved these changes

@engdoreis engdoreis Awaiting requested review from engdoreis

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

Successfully merging this pull request may close these issues.
4 participants
@jwnrt @nbdd0121 @engdoreis @marnovandermaas