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

Requiring re-build after removing ~/.cargo/registry/src #178

Closed
max-sixty opened this issue Jun 21, 2023 · 12 comments
Closed

Requiring re-build after removing ~/.cargo/registry/src #178

max-sixty opened this issue Jun 21, 2023 · 12 comments

Comments

@max-sixty
Copy link

Over at PRQL/prql#2870, we're hitting an issue with how duckdb 0.8.0 uses the cargo cache.

Specifically, the standard cargo cache GHA deletes ~/.cargo/registry/src when it's uploading the cache. But if that's removed, then duckdb-rs requires recompiling. This is the only dependency of ours where this occurs, and it is new after 0.7.1. It also seems to occur in 0.8.1. It's possible to test locally by cargo build, removing the path, and then cargo build again.

It's probably less visible in this repo, since the cargo cache GHA only caches dependencies.

I don't have much context for why this happens — it's very possible it's a dependency of duckdb-rs. It is quite disruptive for our CI — it takes 2-3x longer across most of our jobs.

Would you have any idea for what's causing it? Thank you!

@wangfenjin
Copy link
Collaborator

I think it may related to the way we vendor the duckdb source code, which is the change we made in libduckdb-sys/build.rs

I'll look into that. How should be verify it works if I make any change? I haven't used cargo cache before

@wangfenjin
Copy link
Collaborator

https://github.com/wangfenjin/duckdb-rs/blob/main/libduckdb-sys/build.rs

You mentioned it delete the src/ folder, so as long as we put the file outside src folder it should work fine?

@max-sixty
Copy link
Author

Thanks @wangfenjin !

You mentioned it delete the src/ folder, so as long as we put the file outside src folder it should work fine?

It would need to be in a path that is kept by the cache. The main paths that are retained are paths such as:

./target/x86_64-unknown-linux-gnu/debug/deps/libduckdb-sys-70c2303c3f8e26ab

(I'm less familiar with the differences between the paths in the build; let me know if that's insufficient and I can look further)

@max-sixty
Copy link
Author

I'll look into that. How should be verify it works if I make any change? I haven't used cargo cache before

To test one-off, you can do what I did and remove the ~/.cargo/registry/src and see whether cargo build requires recompiling.

To test systematically I think is difficult — it requires having a project with duckdb as a dependency. I'll take the liberty of tagging @Swatinem if they have ideas...

@Swatinem
Copy link

Thanks for the great analysis, and sorry for the delay here.

I believe this might be an effect of cargo rebuilding things purely based on file timestamps.
In particular, it might be related to these rerun-if-changed, although I haven’t looked in detail:
https://github.com/wangfenjin/duckdb-rs/blob/9f35d497661dee77301620b2289fa53f3de3f377/libduckdb-sys/build.rs#L134-L138

I assume cargo just considers the .crate timestamp for rebuilds, hence you can accidentally edit files inside .cargo/registry/src accidentally when you "go to definition", and cargo not picking up those changes.

It might be the case that cargo will look at the file timestamps of those rerun-if-changed files specifically and see that those have changed, since you are re-extracting those from the .crate files.

Thats just some speculation on my part though. I have seen in particular -sys and build.rs crates not being cached properly, but so far this had negligible effects.

@max-sixty
Copy link
Author

Thanks for replying @Swatinem

I assume cargo just considers the .crate timestamp for rebuilds, hence you can accidentally edit files inside .cargo/registry/src accidentally when you "go to definition", and cargo not picking up those changes.

Would you mind clarifying "go to definition"? I'm familiar with this in an IDE context, but these are tests running in CI?

@Swatinem
Copy link

yes, that was just an observation from local development. In my experience, changing files on disk in the registry/src directory will not lead to rebuilds in local development.

@wangfenjin
Copy link
Collaborator

@max-sixty I deleted ~/.cargo/registry/src and cargo build seems don't requires recompiling.

> rm -rf ~/.cargo/registry/src
> ls ~/.cargo/registry/src
ls: /Users/bytedance/.cargo/registry/src: No such file or directory
> cargo build --features=modern-full,extensions-full
    Finished dev [unoptimized + debuginfo] target(s) in 3.18s

@max-sixty max-sixty mentioned this issue Jul 8, 2023
@max-sixty
Copy link
Author

I assume cargo just considers the .crate timestamp for rebuilds, hence you can accidentally edit files inside .cargo/registry/src accidentally when you "go to definition", and cargo not picking up those changes.

yes, that was just an observation from local development. In my experience, changing files on disk in the registry/src directory will not lead to rebuilds in local development.

OK, I still don't quite get the logic here (I imagine it's me misunderstanding). The issue is that the cache created in CI requires recompilation when it's loaded on the next build. So there are no local files here / no interference from the IDE.

Though maybe that's an explanation of how it doesn't require recompiling locally, only in CI?

@max-sixty
Copy link
Author

@max-sixty I deleted ~/.cargo/registry/src and cargo build seems don't requires recompiling.

Sorry, I realize my instructions weren't specific enough — this indeed doesn't reproduce when in the duckdb-rs repo. Instead, it requires a repo which has duckdb-rs as a dependency, such as https://github.com/PRQL/prql. Running:

cd prql
cargo build --all-features
~/.cargo/registry/src
cargo build --all-features

#  Compiling libduckdb-sys v0.8.1

...causes only libduckdb-sys to recompile.

@max-sixty
Copy link
Author

This is fixed upstream in Swatinem/rust-cache#150, closing.

@max-sixty
Copy link
Author

(fyi I misspoke when I said it was fixed, but will leave this closed regardless)

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

No branches or pull requests

3 participants