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

feat: introduce expander for easier debugging #826

Closed
wants to merge 2 commits into from

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Jun 8, 2024

Attempt to address #760

Comment on lines 408 to 420
let output = if let Some(dest) = std::env::var("PROGENITOR_OUTPUT_DEBUG")
.map(std::path::PathBuf::from_str)
{
eprintln!("Writing generated output to {}", path.display());
expander::Expander::new(dest.filename().expect("Environment var `PROGENITOR_OUTPUT_DEBUG` must contain path with filename"))
.fmt(expander::Edition::_2021)
.verbose(true)
.write_to(output, dest.parent().expect("If path has filename, parent must be root / drive at least. qed"))
.write_to_out_dir(output)
.expect("Writing file works. qed")
} else {
output
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. this doesn't seem to work if there are multiple invocations to generate_api! transitively in the compiled artifact; in fact, the results seem pathological.
  2. I don't know what expander is; let's just use prettyplease (not prettier-please which I can't find the source for).
  3. Let's not change the behavior to include the generated source file.

So:

Suggested change
let output = if let Some(dest) = std::env::var("PROGENITOR_OUTPUT_DEBUG")
.map(std::path::PathBuf::from_str)
{
eprintln!("Writing generated output to {}", path.display());
expander::Expander::new(dest.filename().expect("Environment var `PROGENITOR_OUTPUT_DEBUG` must contain path with filename"))
.fmt(expander::Edition::_2021)
.verbose(true)
.write_to(output, dest.parent().expect("If path has filename, parent must be root / drive at least. qed"))
.write_to_out_dir(output)
.expect("Writing file works. qed")
} else {
output
};
if let Some(dest) = std::env::var("PROGENITOR_OUTPUT_LOG") {
eprintln!("Logging generated output for {} to {}", path_str, dest);
let heading = format!("generated code from {}", path_str);
let output = output.clone();
let file = syn::parse_quote! {
#[doc = "vvvvvvvvvvvvvvvvvvvvvvvvv"]
#[doc = #heading]
#[doc = "^^^^^^^^^^^^^^^^^^^^^"]
#output
};
let text = prettyplease::unparse(&file);
let mut outfile = OpenOptions::new()
.append(true) // Open in append mode
.open("dest")
.map_err(|_| eprintln!("something"));
outfile.write_all(text);
}

Copy link
Contributor Author

@drahnr drahnr Jun 13, 2024

Choose a reason for hiding this comment

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

  1. that's correct, there is the option to use a hash derived from the generated content (see https://docs.rs/expander/latest/src/expander/lib.rs.html#186 ). The proposed solution using PROGENITOR_OUTPUT_LOG suffers the same aliasing issue. Let me re-test, I am pretty sure this already works, other projects use expander that way already.
  2. expander is a solution for rustc not generating good error messages for proc macro generated code, this is the motivation for the indirection. https://github.com/drahnr/expander?tab=readme-ov-file#advantages
  3. The behaviour would only change IFF the user would enable it via an environment variable. I don't see the issue doing the inclusion? Could you elaborate where you see an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. The proposed solution using PROGENITOR_OUTPUT_LOG suffers the same aliasing issue. Let me re-test, I am pretty sure this already works, other projects use expander that way already.

I apologize but I haven't looked at expander and can't prioritize its inclusion to our SBOM.

  1. expander is a solution for rustc not generating good error messages for proc macro generated code, this is the motivation for the indirection. https://github.com/drahnr/expander?tab=readme-ov-file#advantages

Do you have examples of the improved error messages?

  1. The behaviour would only change IFF the user would enable it via an environment variable. I don't see the issue doing the inclusion? Could you elaborate where you see an issue?

When using a debugging facility, I have found that it's a good principle to minimally change the process being debugged. As a concrete example: use of this as-is causes our omicron repo build to fail--much worse than merely imperfect debugging output!

@ahl
Copy link
Collaborator

ahl commented Jun 13, 2024

If you're still interested in moving forward with this without expander, what do you think about having the user specify a directory for generated output rather than a file? We could name the files within that directory based on the input file names.

@ahl ahl closed this Jul 9, 2024
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.

2 participants