Skip to content
This repository was archived by the owner on Feb 28, 2023. It is now read-only.

Include well-known source input dependencies for Rust compilation #51

Merged
merged 1 commit into from
May 17, 2021
Merged

Include well-known source input dependencies for Rust compilation #51

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions src/compiler/rust.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ where
let _dep_info = run_input_output(cmd, None).await?;
// Parse the dep-info file, then hash the contents of those files.
let cwd = cwd.to_owned();
let cwd2 = cwd.to_owned();
let name2 = crate_name.to_owned();
let parsed = pool
.spawn_blocking(move || {
Expand All @@ -237,13 +238,49 @@ where
})
.await?;

parsed.map(move |files| {
trace!(
parsed.map(move |mut files| {
// HACK: Ideally, if we're compiling a Cargo package, we should be
// compiling it with the same files included in the published .crate
// package to ensure maximum compatibility. While it's possible for
// the crate developers to mark files as required for the compilation
// via mechanisms such as `include_bytes!`, this is also a hack and
// may leave an undesired footprint in the compilation outputs.
// An upstream mechanism to only track required additional files is
// pending at https://github.com/rust-lang/rust/pull/84029.
// Until then, unconditionally include Cargo.toml manifest to provide
// a minimal support and to keep some crates compiling that may
// read additional info directly from the manifest file.
let cargo_manifest_dir = env_vars
.iter()
.find(|(key, _)| key.to_str() == Some("CARGO_MANIFEST_DIR"))
.map(|(_, value)| Path::new(value));
const KNOWN_AUX_DEP_FILES: &[(Option<&str>, &str)] = &[
(None, "Cargo.toml"),
(Some("libp2p_wasm_ext"), "src/websockets.js"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are those part of the includes = [] set in the manifest? But on the other handside there might be other rather large files as well 🧐 - I guess it's good as is.
We might want to consider to allow easy modification via env var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has to be included in the packaged and uploaded .crate file to crates.io - otherwise, regular builds wouldn't be possible. The libp2p-wasm-ext has no include/exclude, so by default it includes everything in directory IIRC.
Here's a list of files in the package:

xanewok@faerun-dev:~/repos/rust-libp2p/transports/wasm-ext (master)$ cargo package --list
.cargo_vcs_info.json
CHANGELOG.md
Cargo.toml
Cargo.toml.orig
src/lib.rs
src/websockets.js

We might want to consider to allow easy modification via env var?

Do you mean unconditionally including Cargo.toml for every package?
We have to include src/websockets.js for libp2p-wasm-ext, otherwise we can't ever build it via sccache-dist.

Copy link
Contributor

Choose a reason for hiding this comment

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

It has to be included in the packaged and uploaded .crate file to crates.io - otherwise, regular builds wouldn't be possible. The libp2p-wasm-ext has no include/exclude, so by default it includes everything in directory IIRC.
Here's a list of files in the package:

xanewok@faerun-dev:~/repos/rust-libp2p/transports/wasm-ext (master)$ cargo package --list
.cargo_vcs_info.json
CHANGELOG.md
Cargo.toml
Cargo.toml.orig
src/lib.rs
src/websockets.js

I think that status quo of inclusion logic is good as is :)

We might want to consider to allow easy modification via env var?

Do you mean unconditionally including Cargo.toml for every package?
We have to include src/websockets.js for libp2p-wasm-ext, otherwise we can't ever build it via sccache-dist.

I meant more on the extensability front/generalization i.e. what if another crate pops up in the need of file? We want to have rather quick turnaround, so a CACHEPOT_EXTRA=$crate:$resource_path,.. might be something for a follow up PR (#idea) and I have a feeling that even with the deps tracking this might be a quick fix before it's going to be adopted in the ecosystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that - yeah, that's definitely a good idea. I was thinking of introducing it in a configuration file but env var sounds easy/quicker for now 👍

];
for (target_crate_name, file_path) in KNOWN_AUX_DEP_FILES {
let name_matches = target_crate_name.map_or(true, |x| x == crate_name);
// If this is a Cargo package, try to get the same path prefix as
// other paths are probably using. If, let's say, a package registry
// is symlinked, it might be troublesome for the path transformer to
// correctly figure out which files to include.
let base_dir = cargo_manifest_dir.unwrap_or(&cwd2);
let file_path = base_dir.join(file_path);
if name_matches && file_path.exists() {
files.push(file_path);
}
}
debug!(
"[{}]: got {} source files from dep-info in {}",
crate_name,
files.len(),
fmt_duration_as_secs(&start.elapsed())
);
trace!(
"[{}]: source files calculated from dep-info: {:?}",
crate_name,
files
);
// Just to make sure we capture temp_dir.
drop(temp_dir);
files
Expand Down