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

feat: traverse the target directory instead of intercepting the linker to collect assets #3228

Merged
merged 4 commits into from
Nov 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions packages/cli/src/assets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ impl AssetManifest {
}

/// Fill this manifest with a file object/rlib files, typically extracted from the linker intercepted
pub(crate) fn add_from_object_path(&mut self, path: PathBuf) -> anyhow::Result<()> {
let data = std::fs::read(path.clone())?;
pub(crate) fn add_from_object_path(&mut self, path: &Path) -> anyhow::Result<()> {
let data = std::fs::read(path)?;

match path.extension().and_then(|ext| ext.to_str()) {
// Parse an rlib as a collection of objects
Expand Down
69 changes: 55 additions & 14 deletions packages/cli/src/build/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ use crate::{link::LinkAction, BuildArgs};
use crate::{AppBundle, Platform};
use anyhow::Context;
use serde::Deserialize;
use std::{path::PathBuf, process::Stdio, time::Instant};
use std::{
ffi::OsStr,
path::{Path, PathBuf},
process::Stdio,
time::Instant,
};
use tokio::{io::AsyncBufReadExt, process::Command};

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -60,7 +65,7 @@ impl BuildRequest {
let start = Instant::now();
self.prepare_build_dir()?;
let exe = self.build_cargo().await?;
let assets = self.collect_assets().await?;
let assets = self.collect_assets(&exe).await?;

Ok(BuildArtifacts {
exe,
Expand Down Expand Up @@ -102,9 +107,6 @@ impl BuildRequest {
.args(self.build_arguments())
.envs(self.env_vars()?);

// todo(jon): save the temps into a file that we use for asset extraction instead of the weird double compile.
// .args(["--", "-Csave-temps=y"]);

if let Some(target_dir) = self.custom_target_dir.as_ref() {
cmd.env("CARGO_TARGET_DIR", target_dir);
}
Expand Down Expand Up @@ -203,19 +205,54 @@ impl BuildRequest {
Ok(out_location)
}

/// Run the linker intercept and then fill in our AssetManifest from the incremental artifacts
/// Traverse the target directory and collect all assets from the incremental cache
///
/// This will execute `dx` with an env var set to force `dx` to operate as a linker, and then
/// traverse the .o and .rlib files rustc passes that new `dx` instance, collecting the link
/// tables marked by manganis and parsing them as a ResourceAsset.
pub(crate) async fn collect_assets(&self) -> Result<AssetManifest> {
/// This uses "known paths" that have stayed relatively stable during cargo's lifetime.
/// One day this system might break and we might need to go back to using the linker approach.
pub(crate) async fn collect_assets(&self, exe: &Path) -> Result<AssetManifest> {
tracing::debug!("Collecting assets ...");

if self.build.skip_assets {
return Ok(AssetManifest::default());
}

self.deep_linker_asset_extract().await
// Experimental feature for testing - if the env var is set, we'll use the deeplinker
if std::env::var("DEEPLINK").is_ok() {
tracing::debug!("Using deeplinker instead of incremental cache");
return self.deep_linker_asset_extract().await;
}

// walk every file in the incremental cache dir, reading and inserting items into the manifest.
let mut manifest = AssetManifest::default();

// Add from the deps folder where the rlibs are stored for dependencies
let deps_folder = exe.parent().unwrap().join("deps");
tracing::trace!("Adding assets from deps folder: {deps_folder:?}");
for entry in deps_folder.read_dir()?.flatten() {
if entry.path().extension() == Some(OsStr::new("rlib")) {
_ = manifest.add_from_object_path(&entry.path());
}
}

// Add from the incremental cache folder by recursively walking the folder
// it seems that this sticks around no matter what - and cargo doesn't clean it up since the .os are cached anyway
fn recursive_add(manifest: &mut AssetManifest, path: &Path) -> Result<()> {
if path.extension() == Some(OsStr::new("o")) {
_ = manifest.add_from_object_path(path);
} else if let Ok(dir) = path.read_dir() {
for entry in dir.flatten() {
recursive_add(manifest, &entry.path())?;
}
}

Ok(())
}
recursive_add(&mut manifest, &exe.parent().unwrap().join("incremental"))?;

// And then add from the exe directly, just in case it's LTO compiled and has no incremental cache
_ = manifest.add_from_object_path(exe);

Ok(manifest)
}

/// Create a list of arguments for cargo builds
Expand Down Expand Up @@ -408,6 +445,7 @@ impl BuildRequest {
///
/// There's a chance that's not actually true, so this function is kept around in case we do
/// need to revert to "deep extraction".
#[allow(unused)]
async fn deep_linker_asset_extract(&self) -> Result<AssetManifest> {
// Create a temp file to put the output of the args
// We need to do this since rustc won't actually print the link args to stdout, so we need to
Expand Down Expand Up @@ -619,9 +657,7 @@ impl BuildRequest {
/// a final build output somewhere. Some platforms have basically no build command, and can simply
/// be ran by executing the exe directly.
pub(crate) fn root_dir(&self) -> PathBuf {
let platform_dir = self
.krate
.build_dir(self.build.platform(), self.build.release);
let platform_dir = self.platform_dir();

match self.build.platform() {
Platform::Web => platform_dir.join("public"),
Expand All @@ -639,6 +675,11 @@ impl BuildRequest {
}
}

pub(crate) fn platform_dir(&self) -> PathBuf {
self.krate
.build_dir(self.build.platform(), self.build.release)
}

pub fn asset_dir(&self) -> PathBuf {
match self.build.platform() {
Platform::MacOS => self
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/cli/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ impl LinkAction {
if item.ends_with(".o") || item.ends_with(".rlib") {
let path_to_item = PathBuf::from(item);
if let Ok(path) = path_to_item.canonicalize() {
_ = manifest.add_from_object_path(path);
_ = manifest.add_from_object_path(&path);
}
}
}
Expand Down
Loading