Skip to content

internal: Fix lockfile temp dir usage and use it for build scripts as well #20315

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

Merged
merged 2 commits into from
Jul 27, 2025

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Jul 27, 2025

cc #18508

Didn't notice that in #20290 but it drops the temp dir before the metadata call gets invoked, meaning we never run it against a lockfile

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2025
ProcMacroLoadingError::MissingDylibPath(candidates) => {
write!(
f,
"proc-macro crate built but the dylib path is missing, this indicates a problem with your build system. Candidates not considered: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Candidates considered or not considered?

Copy link
Member Author

@Veykril Veykril Jul 27, 2025

Choose a reason for hiding this comment

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

Not considered, these are files got from rustc/cargo that do not have a dylib extension for the platform. See

// FIXME: Find a better way to know if it is a dylib.
fn is_dylib(path: &Utf8Path) -> bool {
match path.extension().map(|e| e.to_owned().to_lowercase()) {
None => false,
Some(ext) => matches!(ext.as_str(), "dll" | "dylib" | "so"),
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted the message to make that more clear

@Veykril Veykril force-pushed the push-pvmslwwouzzx branch 4 times, most recently from bc7fdae to 3b6aeeb Compare July 27, 2025 15:56
@Veykril
Copy link
Member Author

Veykril commented Jul 27, 2025

Oh haha, I think I now know the issue with the dylib not being found sometimes. The package versions reported by cargo metadata and our build script command sometimes go out of sync because we aren't locking the lockfile I believe

@Veykril
Copy link
Member Author

Veykril commented Jul 27, 2025

@ShoyuVanilla do you know perhaps if --compile-time-deps affects the dependency resolution of cargo? 😅

Nevermind, that's not the issue.

@Veykril Veykril force-pushed the push-pvmslwwouzzx branch 2 times, most recently from ad1374b to ce66520 Compare July 27, 2025 18:27
@Veykril Veykril changed the title internal: Better type proc macro dylib build data state internal: Fix lockfile temp dir usage and use it for build scripts as well Jul 27, 2025
@Veykril Veykril force-pushed the push-pvmslwwouzzx branch from ce66520 to df85aac Compare July 27, 2025 18:28
@Veykril
Copy link
Member Author

Veykril commented Jul 27, 2025

So I think what #18508 observes is that our metadata fetching and build script building may disagree on the exact package ids due to differing lockfiles. I believe this PR should fix that but I am not 100% sure

@Veykril
Copy link
Member Author

Veykril commented Jul 27, 2025

Going to merge this now before our release tomorrow as we otherwise pollute the temp dirs too much and b) compile time deps easily breaking from that

@Veykril Veykril enabled auto-merge July 27, 2025 18:38
@Veykril Veykril added this pull request to the merge queue Jul 27, 2025
Merged via the queue into rust-lang:master with commit 6881029 Jul 27, 2025
15 checks passed
@Veykril Veykril deleted the push-pvmslwwouzzx branch July 27, 2025 18:51
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2025
@Veykril
Copy link
Member Author

Veykril commented Jul 27, 2025

Groan something broke in here silently

@ShoyuVanilla
Copy link
Member

Didn't notice that in #20290 but it drops the temp dir before the metadata call gets invoked, meaning we never run it against a lockfile

Oops, my bad 😅

@ShoyuVanilla do you know perhaps if --compile-time-deps affects the dependency resolution of cargo? 😅

Nevermind, that's not the issue.

FWIW I guess it doesn't because the implementation only affects whether to enqueue a unit compilation job into the job queue or not and it happens after the dependency resolution

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.

4 participants