Skip to content

Fix Create Disk Image Example #300

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

Merged
merged 4 commits into from
Jan 2, 2023

Conversation

AlexJMohr
Copy link
Contributor

@AlexJMohr AlexJMohr commented Dec 22, 2022

The example code didn't compile. This patch fixes that.

Let me know if it needs any changes, and thanks for making this awesome crate!

The example code didn't compile. This patch fixes that.
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Looks like I forgot to try this snippet.

One minor nit: I think the Path::new call is simple enough to do apply it in the out_dir/kernel bindings directly instead of introducing new out_dir_path/kernel_path bindings. I left some suggestions below for this.

@AlexJMohr
Copy link
Contributor Author

AlexJMohr commented Dec 29, 2022

@phil-opp I agree, that looks simpler.

EDIT: now I remember why I did it that way. The compiler complains about temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
  --> build.rs:8:29
   |
8  |     let kernel = Path::new(&std::env::var_os("CARGO_BIN_FILE_KERNEL_kernel").unwrap());
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - temporary value is freed at the end of this statement
   |                             |
   |                             creates a temporary value which is freed while still in use
...
18 |     bootloader::BiosBoot::new(&kernel)
   |                               ------- borrow later used here
   |
help: consider using a `let` binding to create a longer lived value
   |
8  ~     let binding = std::env::var_os("CARGO_BIN_FILE_KERNEL_kernel").unwrap();
9  ~     let kernel = Path::new(&binding);
   |

If you have a different way, let me know

@asensio-project
Copy link
Contributor

Hello, I think that in the Cargo.toml file you should change ovmf_prebuilt to ovmf-prebuilt because the first package doesn't exists in crates.io but the second does.

@phil-opp
Copy link
Member

phil-opp commented Jan 2, 2023

The compiler complains about temporary value is freed at the end of this statement

I think we can use PathBuf instead of Path to take ownership of the string. I pushed 63284c6 with that change.

Hello, I think that in the Cargo.toml file you should change ovmf_prebuilt to ovmf-prebuilt because the first package doesn't exists in crates.io but the second does.

Thanks, done in 0d24cef.

@phil-opp
Copy link
Member

phil-opp commented Jan 2, 2023

Could you try whether the new version works for you? Then we can finally merge this!

@asensio-project
Copy link
Contributor

The new version is working very good, thanks!

@phil-opp
Copy link
Member

phil-opp commented Jan 2, 2023

Ok great, thanks for testing!

@phil-opp phil-opp merged commit 66c1218 into rust-osdev:main Jan 2, 2023
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