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 codegen I/O-free and agnostic to output location #3888

Merged
merged 12 commits into from Oct 17, 2023

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Oct 17, 2023

Commit by commit

This is necessary refactoring work for the upcoming attr.rust.custom_crate attribute, itself necessary for the upcoming serde-codegen support, itself necessary for the upcoming blueprint experimentations as well as #3741.

Changes

  1. The CodeGenerator trait as well as all post-processing helpers (gitattributes, orphan detection...) are now I/O-free.
pub type GeneratedFiles = std::collections::BTreeMap<camino::Utf8PathBuf, String>;

pub trait CodeGenerator {
    fn generate(
        &mut self,
        reporter: &crate::Reporter,
        objects: &crate::Objects,
        arrow_registry: &crate::ArrowRegistry,
    ) -> GeneratedFiles;
}
  1. All post-processing helpers are now agnostic to the location output.
    This is very important as it makes it possible to generate e.g. rust code out of the re_types crate without everything crumbling down.
    A side-effect is that gitattributes files are now finer-grained.
  2. The Rust codegen pass is now crate agnostic: it is driven by the workspace path rather than a specific crate path.
    Necessary for the upcoming attr.rust.custom_crate.
  3. All codegen passes now follow the exact same 4-step structure:
// 1. Generate in-memory code files.
let mut gen = MyGenerator::new();
let mut files = gen.generate(reporter, objects, arrow_registry);
// 2. Generate in-memory attribute files.
generate_gitattributes_for_generated_files(&mut files);
// 3. Write all in-memory files to disk.
write_files(&gen.pkg_path, &gen.testing_pkg_path, &files);
// 4. Remove orphaned files.
crate::codegen::common::remove_orphaned_files(reporter, &files);
  1. The documentation codegen pass now removes its orphans, which is why some md files were removed in this PR.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@emilk emilk self-requested a review October 17, 2023 10:38
crates/re_types_builder/src/codegen/cpp/mod.rs Outdated Show resolved Hide resolved
crates/re_types_builder/src/lib.rs Outdated Show resolved Hide resolved
crates/re_types_builder/src/lib.rs Outdated Show resolved Hide resolved
let stderr = String::from_utf8_lossy(&output.stderr).trim().to_owned();
anyhow::bail!("{stdout}\n{stderr}")
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you moved out the language-specific formatting out of the language-specific modules?

To me it makes more sense if GeneratedFiles would be the final files, with no extra processing required.
Then these functions would be a lot more similar to each other (identical?)

Copy link
Member

Choose a reason for hiding this comment

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

…and it would make the PR diff smaller too :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I just don't like the the mixup of concerns: the codegen pass should only concern itself with generating code, not any post-processing (including formatting) nor I/O.
Especially so with the current implementation where the codegen pass is nice and pure while the formatting pass requires I/O, process spawning, etc.

I will likely introduce a formalized formatting pass in #3495 though, which will make all these generate_XXX methods go away and make --checking trivial.

Copy link
Member

Choose a reason for hiding this comment

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

that makes sense!

@teh-cmc teh-cmc merged commit a364405 into main Oct 17, 2023
30 checks passed
@teh-cmc teh-cmc deleted the cmc/generator_traits branch October 17, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants