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

Evaluate switching to std::path::absolute() once that method is stabilized #30

Closed
simu opened this issue Sep 12, 2023 · 3 comments
Closed
Labels
enhancement New feature or request

Comments

@simu
Copy link
Member

simu commented Sep 12, 2023

Context

Currently, we're using a custom to_lexical_absolute() function (copied from https://internals.rust-lang.org/t/path-to-lexical-absolute/14940) to transform relative paths to their absolute counterparts without following symlinks, cf.

reclass-rs/src/lib.rs

Lines 47 to 69 in 1780735

/// Converts `p` to an absolute path, but doesn't resolve symlinks. The function does normalize the
/// path by resolving any `.` and `..` components which are present.
///
/// Copied from https://internals.rust-lang.org/t/path-to-lexical-absolute/14940.
fn to_lexical_absolute(p: &Path) -> Result<PathBuf> {
let mut absolute = if p.is_absolute() {
PathBuf::new()
} else {
std::env::current_dir()?
};
for component in p.components() {
match component {
Component::CurDir => { /* do nothing for `.` components */ }
Component::ParentDir => {
// pop the last element that we added for `..` components
absolute.pop();
}
// just push the component for any other component
component => absolute.push(component.as_os_str()),
}
}
Ok(absolute)
}

There's discussions around stabilizing std::path::absolute() which is currently available as an unstable feature in nightly Rust. We should check if we can replace our custom to_lexical_absolute() with this method once it's stabilized.

Alternatives

Do nothing and keep maintaining a function that could be replaced by a stdlib method.

@simu simu added the enhancement New feature or request label Sep 12, 2023
@simu
Copy link
Member Author

simu commented May 29, 2024

Looks like std::path::absolute() (which will be stabilized soon) will not resolve /../ segments (apparently in accordance with POSIX path resolution semantics, cf. https://doc.rust-lang.org/std/path/fn.absolute.html. However, this makes the function unsuitable for our usecase.

@ssokolow
Copy link

ssokolow commented Jun 18, 2024

apparently in accordance with POSIX path resolution semantics

% mkdir example
% cd example
% mkdir foo
% touch foo/foo{1..3}
% mkdir bar
% touch bar/bar{1..3}
% mkdir bar/foo
% ls bar/foo/..
bar1  bar2  bar3  foo
% rmdir bar/foo
% ln -s ../foo bar/foo
% ls bar/foo/..
bar  foo

TL;DR: You can't correctly resolve /../ segments without accessing the filesystem because "POSIX path resolution semantics" say its meaning varies depending on the presence or absence of symlinks and that's what the kernel implemented.

@simu
Copy link
Member Author

simu commented Aug 2, 2024

We explicitly don't want to resolve leaf symlinks when determining the name of reclass entities. We already resolve non-leaf symlinks when walking the reclass inventory directory tree, cf.

reclass-rs/src/lib.rs

Lines 121 to 193 in 7b34c22

fn walk_entity_dir(
kind: &EntityKind,
root: &str,
entity_map: &mut HashMap<String, EntityInfo>,
max_depth: usize,
) -> Result<()> {
let entity_root = to_lexical_absolute(&PathBuf::from(root))?;
// We need to follow symlinks when walking the root directory, so that inventories which
// contain symlinked directories are loaded correctly.
for entry in WalkDir::new(root).max_depth(max_depth).follow_links(true) {
let entry = entry?;
// We use `entry.path()` here to get the symlink name for symlinked files.
let ext = if let Some(ext) = entry.path().extension() {
ext.to_str()
} else {
None
};
if ext.is_some() && SUPPORTED_YAML_EXTS.contains(&ext.unwrap()) {
// it's an entity (class or node), process it
let abspath = to_lexical_absolute(entry.path())?;
let relpath = abspath.strip_prefix(&entity_root)?;
let cls = relpath.with_extension("");
let (cls, loc) = if cls.ends_with("init") {
// treat `foo/init.yml` as contents for class `foo`
let cls = cls
.parent()
.ok_or(anyhow!(
"Failed to normalize entity {}",
entry.path().display()
))?
.to_owned();
// here, unwrap can't panic since we otherwise would have already returned an error
// in the previous statement.
let loc = relpath.parent().unwrap();
// For `init.ya?ml` classes, the location is parent directory of the directory
// holding the class file.
(cls, loc.parent().unwrap_or(Path::new("")))
} else {
// For normal classes, the location is the directory holding the class file.
(cls, relpath.parent().unwrap_or(Path::new("")))
};
let cls = cls.to_str().ok_or(anyhow!(
"Failed to normalize entity {}",
entry.path().display()
))?;
let (cls, loc) = if kind == &EntityKind::Node && max_depth > 1 && cls.starts_with('_') {
// special case node paths starting with _ for compose-node-name
(
cls.split(MAIN_SEPARATOR).last().ok_or(anyhow!(
"Can't shorten node name for {}",
entry.path().display()
))?,
Path::new(""),
)
} else {
(cls, loc)
};
let cls = cls.replace(MAIN_SEPARATOR, ".");
if let Some(prev) = entity_map.get(&cls) {
return err_duplicate_entity(kind, root, relpath, &cls, &prev.path);
}
entity_map.insert(
cls,
EntityInfo {
path: relpath.to_path_buf(),
loc: PathBuf::from(loc),
},
);
}
}
Ok(())
}

@simu simu closed this as completed Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants