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

Treat dependencies of proc-macro crates like normal dependencies #69976

Closed
wants to merge 4 commits into from
Closed
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
32 changes: 23 additions & 9 deletions src/bootstrap/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -838,12 +838,12 @@ pub fn run_cargo(
let mut deps = Vec::new();
let mut toplevel = Vec::new();
let ok = stream_cargo(builder, cargo, tail_args, &mut |msg| {
let (filenames, crate_types) = match msg {
let (filenames, crate_types, kind, name) = match msg {
CargoMessage::CompilerArtifact {
filenames,
target: CargoTarget { crate_types },
target: CargoTarget { crate_types, kind, name },
..
} => (filenames, crate_types),
} => (filenames, crate_types, kind, name),
_ => return,
};
for filename in filenames {
Expand All @@ -859,12 +859,24 @@ pub fn run_cargo(

let filename = Path::new(&*filename);

// If this was an output file in the "host dir" we don't actually
// worry about it, it's not relevant for us
// If this was an output file in the "host dir", we want
// to ignore it if it's a build script.
if filename.starts_with(&host_root_dir) {
// Unless it's a proc macro used in the compiler
if crate_types.iter().any(|t| t == "proc-macro") {
deps.push((filename.to_path_buf(), true));
deps.push((filename.to_path_buf(), /* host_dep */ true));
}

let is_build_script = kind == &["custom_build"]
&& crate_types == &["bin"]
&& name == "build-script-build";
// We don't care about build scripts, but we *do* care about proc-macro
// dependencies - the compiler needs their metadata when loading proc-macro
// crates. Anything that's not a build script should be a proc-macro dependency.
//
// FIXME: Have Cargo explicitly indicate build-script vs proc-macro dependencies,
// instead of relying on this check.
if !is_build_script {
deps.push((filename.to_path_buf(), /* host_dep */ true));
}
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need a different check from the above crate_types.iter().any(|t| t == "proc-macro")?

Generally it also feels wrong or at least odd that we're putting proc macros seemingly twice, once into the host and once into the target directory. That feels reminiscent of a flag @Zoxc, IIRC, added (-Zdual-macros, or so, I don't recall exactly).

Copy link
Member Author

Choose a reason for hiding this comment

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

The crate_types.iter().any(|t| t == "proc-macro") check is looking for proc macro crates. This check is looking for dependencies of proc-macros, which may not be proc-macros themselves.

For example, suppose we have the dependency graph normal_crate_one -> proc_macro_crate -> normal_crate_two. The crate normal_crate_two is not a proc macro crate - but since it's a dependency of proc_macro_crate, it will end up in the host dir.

continue;
}
Expand Down Expand Up @@ -933,8 +945,8 @@ pub fn run_cargo(
deps.extend(additional_target_deps.into_iter().map(|d| (d, false)));
deps.sort();
let mut new_contents = Vec::new();
for (dep, proc_macro) in deps.iter() {
new_contents.extend(if *proc_macro { b"h" } else { b"t" });
for (dep, host_dep) in deps.iter() {
new_contents.extend(if *host_dep { b"h" } else { b"t" });
new_contents.extend(dep.to_str().unwrap().as_bytes());
new_contents.extend(b"\0");
}
Expand Down Expand Up @@ -1003,6 +1015,8 @@ pub fn stream_cargo(
#[derive(Deserialize)]
pub struct CargoTarget<'a> {
crate_types: Vec<Cow<'a, str>>,
kind: Vec<Cow<'a, str>>,
name: Cow<'a, str>,
}

#[derive(Deserialize)]
Expand Down
7 changes: 4 additions & 3 deletions src/librustc_metadata/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,9 +558,10 @@ impl<'a> CrateLoader<'a> {
dep_kind: DepKind,
) -> CrateNumMap {
debug!("resolving deps of external crate");
if crate_root.is_proc_macro_crate() {
return CrateNumMap::new();
}

// Note that we need to resolve deps for proc-macro crates (just like normal crates)
// since we may need to decode `Span`s that reference the `CrateNums`
// of transitive dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this comment looks unnecessary, like "look, nothing unusual is happening here, move along". It's the removed special case that required a comment, but didn't have it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We still special-case proc macros is some places, so I thought it was worthwhile to explain why we do this (in case someone ever thinks of re-adding this an an optimization).

Copy link
Member

Choose a reason for hiding this comment

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

There's nothing here about proc macros anymore though? I would agree with @petrochenkov that this comment seems more confusing than helpful. We should have a test that ensures this doesn't get re-added?


// The map from crate numbers in the crate we're resolving to local crate numbers.
// We map 0 and all other holes in the map to our parent crate. The "additional"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
'rsbegin.o', 'rsend.o', 'dllcrt2.o', 'crt2.o', 'clang_rt']


# This is a whitelist of crates which give an "compiled by an incompatible version of rustc"
# error, due to being copied into the sysroot as proc-macro deps. See #69976
INCOMPATIBLE_VER_CRATES = ['autocfg', 'cc']

def convert_to_string(s):
if s.__class__.__name__ == 'bytes':
return s.decode('utf-8')
Expand All @@ -34,6 +38,11 @@ def check_lib(lib):
stdout, stderr = exec_command([os.environ['RUSTC'], '-', '--crate-type', 'rlib',
'--extern', '{}={}'.format(lib['name'], lib['path'])],
to_input=('extern crate {};'.format(lib['name'])).encode('utf-8'))

if 'compiled by an incompatible version of rustc' in '{}{}'.format(stdout, stderr):
if lib['name'] in INCOMPATIBLE_VER_CRATES:
return True

if not 'use of unstable library feature' in '{}{}'.format(stdout, stderr):
print('crate {} "{}" is not unstable'.format(lib['name'], lib['path']))
print('{}{}'.format(stdout, stderr))
Expand Down