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

Fixed typos regarding .cargo/config.toml #337

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

rdelfin
Copy link
Contributor

@rdelfin rdelfin commented May 11, 2021

The file keeps on being referred to as either cargo/config.toml or .cargo/config.toml. This PR corrects all of them to .cargo/config.toml

The file keeps on being referred to as either `cargo/config.toml` or `.cargo/config.toml`. This PR corrects all of them to `.cargo/config.toml`
@rdelfin rdelfin requested a review from a team as a code owner May 11, 2021 18:10
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jamesmunns (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources labels May 11, 2021
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Thanks!

bors merge

bors bot added a commit that referenced this pull request May 11, 2021
337: Fixed typos regarding `.cargo/config.toml` r=adamgreig a=rdelfin

The file keeps on being referred to as either `cargo/config.toml` or `.cargo/config.toml`. This PR corrects all of them to `.cargo/config.toml`

Co-authored-by: Ricardo Delfin <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 11, 2021

Build failed:

@adamgreig
Copy link
Member

bors retry

bors bot added a commit that referenced this pull request May 11, 2021
337: Fixed typos regarding `.cargo/config.toml` r=adamgreig a=rdelfin

The file keeps on being referred to as either `cargo/config.toml` or `.cargo/config.toml`. This PR corrects all of them to `.cargo/config.toml`

Co-authored-by: Ricardo Delfin <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 11, 2021

Build failed:

@adamgreig
Copy link
Member

Huh, that's more worrying. Not your fault though, will investigate.

@rdelfin
Copy link
Contributor Author

rdelfin commented May 12, 2021

The wonders of CI

@sirhcel
Copy link
Contributor

sirhcel commented May 19, 2021

Huh, that's more worrying. Not your fault though, will investigate.

It looks like depending on two panic handlers (panic-halt and panic-itm) works fine when compiling the example code (as it actually only uses panic-itm). But it also looks like cargo doc does not take the actual usage into account and fails due to the duplicated panic_impl.

Removing the dependencies on panic-halt fixes building the documentation for me (sirhcel@3e2a180).

@eldruin eldruin mentioned this pull request May 25, 2021
@sirhcel
Copy link
Contributor

sirhcel commented May 26, 2021

@adamgreig and @rdelfin is there anything speaking against removing panic-halt to fix building the book? I don't see a reason why the dependency to panic-halt was there in the first place.

In case you have no objections how to proceed? Should I create a PR against your branch @rdelfin? Our would you simply pick the commit?

@adamgreig
Copy link
Member

If that's enough to get it working please do go ahead with a PR, probably against master is easiest since we can merge that first and then when we retry these other PRs, they will be tested against the merged version.

@rdelfin
Copy link
Contributor Author

rdelfin commented May 27, 2021

I'd be glad to rebase on top of that change after it merges, given that's probably the easiest option!

@sirhcel
Copy link
Contributor

sirhcel commented May 27, 2021

I'm sorry, but it looks like I had a mental blackout late at night when writing #337 (comment). My proposal fixes cargo doc but none of the actual issues discussed here. I'm really sorry for bugging you! :(

#267 and #341 already propose actually suitable changes and you already pointed out in the latter that there is some real work ahead @adamgreig. I will have a look if I could contribute something to these PRs.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

This should now work after the latest fixes.

bors r+

@bors bors bot merged commit 8a18ebd into rust-embedded:master Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants