Skip to content

feat: checksum manifest file in forc-pkg #7172

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

kayagokalp
Copy link
Member

@kayagokalp kayagokalp commented May 13, 2025

Description

This is the first of possibly many PRs to get package level caching. I decided to split this into pieces after trying to get everything done at one go.

This one basically adds checksum implementation for workspace and package level manifest files. Taking the following into account:

  1. The dependencies and their versions (without taking the actual string representation in to account to prevent things like different checksums for forc.tomls with different order of dependencies)
  2. The state of the all .sw files in the src folder

To give an example how to use this in the LSP:

  1. Create checksum of the pkgs open in the LSP, hold it in the memory
  2. If it is a single package this is simply a checksum string value, to decide whether to build it again or not, we can check the checksum again.
  3. If it is a workspace we might want to hold a ChecksumGraph in the forc-pkg side. This will enable us to pin-point which exact package is different.

Once we are able to serialize the compiled packages, and the engines to the disk, this can be used a primitive form of incremental building.

TODO

  • Create ChecksumGraph on forc-pkg side, and expose a function for consumers of forc-pkg to create this ChecksumGraph
  • Consider creating the checksum based on the AST so that simple whitespace changes do not effect us

@kayagokalp kayagokalp requested review from a team as code owners May 13, 2025 20:55
@kayagokalp kayagokalp self-assigned this May 13, 2025
@kayagokalp kayagokalp added enhancement New feature or request forc-pkg Everything related to the `forc-pkg` crate. labels May 13, 2025
Copy link

codspeed-hq bot commented May 13, 2025

CodSpeed Performance Report

Merging #7172 will not alter performance

Comparing kayagokalp/7142 (d55d7f8) with master (be7a969)

Summary

✅ 22 untouched benchmarks

Copy link
Member

Choose a reason for hiding this comment

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

do we need to check in an empty file?

.to_string_lossy()
.to_string();

println!("adding {:?}", rel_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this for prod?
If so; should we use the tracing libs print function(s)?


println!("adding {:?}", rel_path);
// Read file contents
let content = std::fs::read(path).map_err(|e| {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Consider streaming each file into the hasher - instead of doing std::fs::read.

This skips the full-file Vec<u8> allocation and the extra copy into the hasher’s 64-byte buffer.
It barely matters for small projects, but on large workspaces the heap grows with total source size, so streaming can cut both memory and copy time.
Minimal example of streaming bytes from file:

fn hash_file(hasher: &mut Sha256, path: &std::path::Path) -> io::Result<()> {
    let mut reader = BufReader::new(File::open(path)?); // handle to the file; not read into heap
    let mut buf = [0u8; 8 * 1024];           // reusable 8 KiB stack buffer
    loop {
        let n = reader.read(&mut buf)?;
        if n == 0 { break; }
        hasher.update(&buf[..n]);            // stream chunk directly
    }
    Ok(())
}

Note: Can be another PR however (GH issue).

zees-dev
zees-dev previously approved these changes May 14, 2025
Copy link
Contributor

@zees-dev zees-dev left a comment

Choose a reason for hiding this comment

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

LGTM; thorough set of tests to validate functionality!

Copy link
Contributor

@zees-dev zees-dev left a comment

Choose a reason for hiding this comment

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

reverting approval until minor comments (non-nits) are addressed*

@zees-dev zees-dev self-requested a review May 14, 2025 04:31
@zees-dev zees-dev dismissed their stale review May 14, 2025 04:31

reverting approval until minor comments are addressed*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request forc-pkg Everything related to the `forc-pkg` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants