Skip to content

Commit

Permalink
fix(package): warn if symlinks checked out as plain text files (#14994)
Browse files Browse the repository at this point in the history
### What does this PR try to resolve?

`cargo package` will warn users when git `core.symlinks` is `false`
and some symlinks were checked out as plain files during packaging.

Git config [`core.symlinks`] defaults to true when unset.
In git-for-windows (and git as well),
the config should be set to false explicitly when the repo was created,
if symlink support wasn't detected [^1].

We assume the config was always set at creation time and never changed.
So, if it is true, we don't bother users with any warning.

[^1]:
https://github.com/git-for-windows/git/blob/f1241afcc7956918d5da33ef74abd9cbba369247/setup.c#L2394-L2403

[`core.symlinks`]:
https://git-scm.com/docs/git-config#Documentation/git-config.txt-coresymlinks

### How should we test and review this PR?

CI passes.

This shares two commits 42dc4ef and
c8c8223 with #14981.

I didn't commit to fix all symlink issues all at once.
This PR demonstrates how we could leverage metadata in `PathEntry`.
Maybe later we can really follow plain-text symlinks and resolve the
issue.

### Additional information

cc #5664
  • Loading branch information
epage authored Dec 31, 2024
2 parents 0499e31 + 059fe16 commit d73d2ca
Show file tree
Hide file tree
Showing 6 changed files with 295 additions and 18 deletions.
5 changes: 3 additions & 2 deletions src/cargo/ops/cargo_package/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::core::Workspace;
use crate::core::{Package, PackageId, PackageSet, Resolve, SourceId};
use crate::ops::lockfile::LOCKFILE_NAME;
use crate::ops::registry::{infer_registry, RegistryOrIndex};
use crate::sources::path::PathEntry;
use crate::sources::registry::index::{IndexPackage, RegistryDependency};
use crate::sources::{PathSource, CRATES_IO_REGISTRY};
use crate::util::cache_lock::CacheLockMode;
Expand Down Expand Up @@ -396,7 +397,7 @@ fn prepare_archive(
fn build_ar_list(
ws: &Workspace<'_>,
pkg: &Package,
src_files: Vec<PathBuf>,
src_files: Vec<PathEntry>,
vcs_info: Option<vcs::VcsInfo>,
) -> CargoResult<Vec<ArchiveFile>> {
let mut result = HashMap::new();
Expand All @@ -420,7 +421,7 @@ fn build_ar_list(
.push(ArchiveFile {
rel_path: rel_path.to_owned(),
rel_str: rel_str.to_owned(),
contents: FileContents::OnDisk(src_file.clone()),
contents: FileContents::OnDisk(src_file.to_path_buf()),
});
}
}
Expand Down
54 changes: 52 additions & 2 deletions src/cargo/ops/cargo_package/vcs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use serde::Serialize;
use tracing::debug;

use crate::core::Package;
use crate::sources::PathEntry;
use crate::CargoResult;
use crate::GlobalContext;

Expand Down Expand Up @@ -41,7 +42,7 @@ pub struct GitVcsInfo {
#[tracing::instrument(skip_all)]
pub fn check_repo_state(
p: &Package,
src_files: &[PathBuf],
src_files: &[PathEntry],
gctx: &GlobalContext,
opts: &PackageOpts<'_>,
) -> CargoResult<Option<VcsInfo>> {
Expand Down Expand Up @@ -91,6 +92,8 @@ pub fn check_repo_state(
return Ok(None);
}

warn_symlink_checked_out_as_plain_text_file(gctx, src_files, &repo)?;

debug!(
"found (git) Cargo.toml at `{}` in workdir `{}`",
path.display(),
Expand All @@ -110,11 +113,57 @@ pub fn check_repo_state(
return Ok(Some(VcsInfo { git, path_in_vcs }));
}

/// Warns if any symlinks were checked out as plain text files.
///
/// Git config [`core.symlinks`] defaults to true when unset.
/// In git-for-windows (and git as well),
/// the config should be set to false explicitly when the repo was created,
/// if symlink support wasn't detected [^1].
///
/// We assume the config was always set at creation time and never changed.
/// So, if it is true, we don't bother users with any warning.
///
/// [^1]: <https://github.com/git-for-windows/git/blob/f1241afcc7956918d5da33ef74abd9cbba369247/setup.c#L2394-L2403>
///
/// [`core.symlinks`]: https://git-scm.com/docs/git-config#Documentation/git-config.txt-coresymlinks
fn warn_symlink_checked_out_as_plain_text_file(
gctx: &GlobalContext,
src_files: &[PathEntry],
repo: &git2::Repository,
) -> CargoResult<()> {
if repo
.config()
.and_then(|c| c.get_bool("core.symlinks"))
.unwrap_or(true)
{
return Ok(());
}

if src_files.iter().any(|f| f.maybe_plain_text_symlink()) {
let mut shell = gctx.shell();
shell.warn(format_args!(
"found symbolic links that may be checked out as regular files for git repo at `{}`\n\
This might cause the `.crate` file to include incorrect or incomplete files",
repo.workdir().unwrap().display(),
))?;
let extra_note = if cfg!(windows) {
"\nAnd on Windows, enable the Developer Mode to support symlinks"
} else {
""
};
shell.note(format_args!(
"to avoid this, set the Git config `core.symlinks` to `true`{extra_note}",
))?;
}

Ok(())
}

/// The real git status check starts from here.
fn git(
pkg: &Package,
gctx: &GlobalContext,
src_files: &[PathBuf],
src_files: &[PathEntry],
repo: &git2::Repository,
opts: &PackageOpts<'_>,
) -> CargoResult<Option<GitVcsInfo>> {
Expand All @@ -136,6 +185,7 @@ fn git(
let mut dirty_src_files: Vec<_> = src_files
.iter()
.filter(|src_file| dirty_files.iter().any(|path| src_file.starts_with(path)))
.map(|p| p.as_ref())
.chain(dirty_metadata_paths(pkg, repo)?.iter())
.map(|path| {
pathdiff::diff_paths(path, cwd)
Expand Down
4 changes: 3 additions & 1 deletion src/cargo/ops/vendor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::core::shell::Verbosity;
use crate::core::{GitReference, Package, Workspace};
use crate::ops;
use crate::sources::path::PathSource;
use crate::sources::PathEntry;
use crate::sources::CRATES_IO_REGISTRY;
use crate::util::cache_lock::CacheLockMode;
use crate::util::{try_canonicalize, CargoResult, GlobalContext};
Expand Down Expand Up @@ -315,13 +316,14 @@ fn sync(
fn cp_sources(
pkg: &Package,
src: &Path,
paths: &[PathBuf],
paths: &[PathEntry],
dst: &Path,
cksums: &mut BTreeMap<String, String>,
tmp_buf: &mut [u8],
gctx: &GlobalContext,
) -> CargoResult<()> {
for p in paths {
let p = p.as_ref();
let relative = p.strip_prefix(&src).unwrap();

match relative.to_str() {
Expand Down
1 change: 1 addition & 0 deletions src/cargo/sources/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
pub use self::config::SourceConfigMap;
pub use self::directory::DirectorySource;
pub use self::git::GitSource;
pub use self::path::PathEntry;
pub use self::path::PathSource;
pub use self::path::RecursivePathSource;
pub use self::registry::{
Expand Down
Loading

0 comments on commit d73d2ca

Please sign in to comment.