-
Notifications
You must be signed in to change notification settings - Fork 314
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
pkg export and hab-sup/hab-launcher commands should honor fs_root #5592
Conversation
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A suggestion to make the code more understandable, which you can use or not.
components/hab/src/exec.rs
Outdated
@@ -77,8 +77,8 @@ where | |||
return Err(Error::ExecCommandNotFound(command)); | |||
} | |||
|
|||
let fs_root_path = Path::new("/"); | |||
match PackageInstall::load_at_least(ident, None) { | |||
let fs_root_path = Path::new(&*FS_ROOT_PATH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
let fs_root_path = FS_ROOT_PATH.as_path();
I think doing this more explicitly is easier to understand for those who may not intuitively understand all of the rust magic going on here. Especially when the name FS_ROOT_PATH
doesn't make it clear that its type is actually a reference to a PathBuf
.
let fs_root_path = &*FS_ROOT_PATH;
Works too, but I think that while it's shorter, it's more confusing.
@@ -605,8 +606,9 @@ fn supervisor_cmd() -> Result<PathBuf> { | |||
return Ok(PathBuf::from(command)); | |||
} | |||
let ident = PackageIdent::from_str(SUP_PACKAGE_IDENT).unwrap(); | |||
match PackageInstall::load_at_least(&ident, None) { | |||
Ok(install) => match core::fs::find_command_in_pkg(SUP_CMD, &install, "/") { | |||
let fs_root_path = Path::new(&*FS_ROOT_PATH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment
Signed-off-by: mwrock <matt@mattwrock.com>
Obvious fix; these changes are the result of automation not creative thinking.
This week @trevorghess noticed that
hab pkg export docker
inside of a local windows studio was failing with:After researching this it ends up that
hab pkg export
commands do not honor theFS_ROOT
environment variable on Windows which will cause export to break in a local (non-docker) studio.This fixes that scenario. It also fixes another case I have been aware of for a while but was not broken enough to research. Formerly
hab-launch
andhab-sup
binary calls have not honored fs_root either. This means that in a local Windows studio, the launcher and supervisor binaries inc:\hab\pkgs
are invoked. Functionally that is really not a big deal but is out of alignment with the self contained nature of a studio.Signed-off-by: mwrock matt@mattwrock.com