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

Fix Rust build caching by using absolute paths #45

Closed

Conversation

michaelsproul
Copy link
Contributor

We've had Lighthouse users on Discord reporting linking errors related to blst for a while, which would always be solved by cargo clean and a rebuild (suggesting a build caching issue). Today it happened to me and I decided to investigate.

I found that the build script's use of ../.. was causing the C compiler to reach out of the sandbox (OUT_DIR) given to it by Cargo, and into a path shared by all Cargo packages that shouldn't be modified from a build script. One of the compiler invocations it was making was:

running: "cc" "-O2" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-Wall" "-Wextra" "-mno-avx" "-Wno-unused-command-line-argument" "-D__ADX__" "-o" "/home/michael/Programming/lighthouse3/target/release/build/blst-b4fcc12bd32c5abb/out/../../src/server.o" "-c" "../../src/server.c"

Note the ../.. in the -o argument that causes files to be written to target/release/build/src/server.o. If there are multiple versions of blst in the build cache (due to updates, etc), then they share these files, and I think that must confuse Cargo (unfortunately I can't describe an exact mechanism of action here). The Cargo docs have this to say:

Build scripts may save any output files in the directory specified in the OUT_DIR environment variable. Scripts should not modify any files outside of that directory.

In an attempt to fix it, I've switched to using absolute paths, and can confirm that the compiler invocations look more sensible now:

running: "cc" "-O2" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-Wall" "-Wextra" "-mno-avx" "-Wno-unused-command-line-argument" "-D__ADX__" "-o" "/home/michael/Programming/blst/bindings/rust/target/release/build/blst-aa4544c2724aab9e/out/server.o" "-c" "/home/michael/Programming/blst/src/server.c"

And server.o ends up in OUT_DIR instead of its previous location. I hope this is the only build caching issue, and alleviates the problems our users have been facing 🤞

@michaelsproul
Copy link
Contributor Author

I deleted the commented out code for bindgen, as that seems to be covered by build.sh now, but I'm happy to put it back if you'd like to keep it.

I contemplated deleting the override mechanism for local libblast.a and blst, but figured they were harmless because we never use them downstream, and they may be useful for you.

@dot-asm
Copy link
Collaborator

dot-asm commented Nov 27, 2020

Cool! Thanks! I took liberty to align commentary with rustfmt.toml. And it also allowed to clean up CI scripts... In other words, merged. Thanks again:-)

@dot-asm dot-asm closed this Nov 27, 2020
@michaelsproul
Copy link
Contributor Author

Awesome, thanks!

@michaelsproul michaelsproul deleted the rust-build-caching branch November 27, 2020 12:24
bors bot pushed a commit to sigp/lighthouse that referenced this pull request Dec 3, 2020
## Issue Addressed

Should resolve `blst` build issues that previously required `cargo clean` 🤞

## Proposed Changes

BLST cleaned up some of their validation logic: supranational/blst@v0.3.1...v0.3.2

And included my build system PR: supranational/blst#45
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