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

remove runtime crate #928

Merged
merged 1 commit into from
Jan 24, 2024
Merged

remove runtime crate #928

merged 1 commit into from
Jan 24, 2024

Conversation

leonardoalt
Copy link
Member

For the tests that use crates it's possible to use the same powdr-riscv-rt crate from the checkout. For file tests we need to use git. I kept them separate for now but I can make everything git if that's preferred.

riscv/src/lib.rs Outdated
@@ -152,7 +152,7 @@ version = "0.1.0"
edition = "2021"

[dependencies]
runtime = {{ path = "./runtime" }}
powdr_riscv_rt = {{ git = "https://github.com/powdr-labs/powdr", branch = "main" }}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this also need the runtime subdirectory?

Copy link
Member Author

Choose a reason for hiding this comment

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

What for?

@@ -4,6 +4,6 @@ version = "0.1.0"
edition = "2021"

[dependencies]
runtime = { path = "../../../runtime" }
powdr-riscv-rt = { path = "../../../../powdr_riscv_rt" }
Copy link
Member

Choose a reason for hiding this comment

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

I would opt for actually spelling runtime out. rt can mean so many things.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is rather common. We use another crate called riscv_rt for qemu, so i thought powdr-riscv-rt fits well

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, common also if you include the context you get from the "riscv" prefix to "rt"

Copy link
Member

Choose a reason for hiding this comment

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

Even if it's common, I really don't think it's a good idea. We also don't abbreviate executor or other stuff.

Copy link
Member

Choose a reason for hiding this comment

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

I mean isn't even runtime already an abbreviation for runtime environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I still have a somewhat strong opinion about this. powdr-riscv-runtime is just really long and kinda clumsy. If someone else says they prefer it spelled out I'll give in.

Copy link
Member

Choose a reason for hiding this comment

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

What does it matter if it's long and clumsy? Do you need to type it that often?

Copy link
Member Author

Choose a reason for hiding this comment

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

just aesthetics

@leonardoalt leonardoalt changed the base branch from main to rename-crates January 23, 2024 18:52
Base automatically changed from rename-crates to main January 23, 2024 19:30
@leonardoalt leonardoalt changed the base branch from main to rename-riscv-rt January 23, 2024 19:44
Base automatically changed from rename-riscv-rt to main January 23, 2024 21:08
@leonardoalt leonardoalt added this pull request to the merge queue Jan 24, 2024
Merged via the queue into main with commit 655cd1c Jan 24, 2024
2 checks passed
@leonardoalt leonardoalt deleted the remove-runtime branch January 24, 2024 11:47
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.

2 participants