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

Make source links relative to OUT_DIR #9580

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cranelift/codegen/meta/src/srcgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ impl Formatter {
directory: &std::path::Path,
) -> Result<(), error::Error> {
let path = directory.join(&filename);
eprintln!("Writing generated file: {}", path.display());
let mut f = fs::File::create(path)?;

for l in self.lines.iter().map(|l| l.as_bytes()) {
Expand Down
2 changes: 1 addition & 1 deletion cranelift/isle/isle/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl<'a> Codegen<'a> {
"// Generated automatically from the instruction-selection DSL code in:",
)
.unwrap();
for file in &self.files.file_names {
for file in &self.files.file_names_relative() {
writeln!(code, "// - {file}").unwrap();
}

Expand Down
21 changes: 21 additions & 0 deletions cranelift/isle/isle/src/files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,18 @@ impl Files {
self.file_names.get(file).map(|x| x.as_str())
}

/// Same as `file_name` but try to make the file relative to the project root. Otherwise,
/// return the original file name (if found).
pub fn file_name_relative(&self, file: usize) -> Option<&str> {
self.file_name(file).map(|f| relative(f))
}

/// Try to make file names relative to the project root. Otherwise, return the original file
/// names.
pub fn file_names_relative(&self) -> Vec<&str> {
self.file_names.iter().map(|f| relative(f)).collect()
}

pub fn file_text(&self, file: usize) -> Option<&str> {
self.file_texts.get(file).map(|x| x.as_str())
}
Expand All @@ -109,6 +121,15 @@ impl Files {
}
}

/// If OUT_DIR is set, strips it from the file name.
/// Otherwise returns the original file name.
pub fn relative(file_name: &str) -> &str {
match std::env::var("OUT_DIR") {
Err(_) => file_name,
Ok(root) => file_name.strip_prefix(&root).unwrap_or(file_name).into(),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the build script that invokes cranelift-isle can read this instead. Cranelift-isle can technically be used to build arbitrary code. Files::from_paths could get a set of paths to make make input paths relative to, or alternatively a set of paths against which to resolve relative paths and then have the build script only pass the relative paths for individual isle files instead.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, not too familiar with the code/cranelift. Are you suggesting that we should make the paths relative here instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. There is already a make_isle_source_path_relative function which gets used, but that only affects isle files that are part of the source rather than those that get generated by the build script itself. You did still have to separately pass a (set of) prefix(s) to join with the relative path to get a path that can be passed to fs::read, but this prefix can be ignored when printing the source locations in the generated file.

Copy link
Author

Choose a reason for hiding this comment

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

only affects isle files that are part of the source rather than those that get generated by the build script itself.

Unrelated to this thread, but that explains why I had to use OUT_DIR instead of CARGO_MANIFEST_DIR !

Yes.

That makes sense. I'll try to find some time later to update the PR.


#[cfg(test)]
mod tests {
use super::*;
Expand Down
2 changes: 1 addition & 1 deletion cranelift/isle/isle/src/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ impl Pos {
pub fn pretty_print_line(&self, files: &Files) -> String {
format!(
"{} line {}",
files.file_name(self.file).unwrap(),
files.file_name_relative(self.file).unwrap(),
files.file_line_map(self.file).unwrap().line(self.offset)
)
}
Expand Down