Skip to content

fix: library mapping in linking checks #1721

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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

zelosleone
Copy link
Collaborator

closes #1684

now we will:

  • populate library mappings after package installation
  • hopefully do more accurate detection of system vs conda-provided libraries but i will wait for wolf's opinion on this
  • ensure cache dependencies are also properly checked

since library mapping is also improved, now we will be able to track files provided by each package as well!

@zelosleone zelosleone requested a review from wolfv June 11, 2025 21:27
@wolfv
Copy link
Member

wolfv commented Jun 17, 2025

IMO this is too much change in a single PR.

What I would be fine with is if we change the overlinking checks to not rely on the host prefix files but instead combine the packages from host prefix & cache host prefix and determine the shared libraries based on the paths.json file from the different packages (without touching the actual filesystem).

@@ -328,6 +417,73 @@ pub fn perform_linking_checks(
continue;
}

// Use library mapping to find which package provides this library
if let Some(lib_name) = lib.file_name().and_then(|n| n.to_str()) {
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit unsure about this code. What was changed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have basically two steps for library checking, one was where we would populate the library mapping and you recommended we only write it once, the second one is this. perform_linking_checks's main goal is basically to run after main build succeeds to produce new binaries and:

  • look at only the new files that are produced, parse their library names and classify them with library_mapping.get(name)

the goal is to check whatever our new produced binaries are linked against existing libraries, so while step 1 looks into the existing ones this piece of code you mentioned is on the second step where we are looking through newly built ones. so its like this function will use the table we create in the first step and match the newly built binaries. but i could go with a different approach if you want, i thought long and hard about this and this was the best i could come up with (i tried to think about reducing this to just one step and maybe go with that but i dont think that would cover every case)

@wolfv
Copy link
Member

wolfv commented Jun 24, 2025

I am thinking the best thing would be to add a new file in the cache folder that would capture these mappings (or let the first post-processing step create this file).

We could make a new file that would be called something like "post-process-mappings.json" and place it in the $CACHE_FOLDER/post-process-mappings.json.

Then, every process using the cache contents could also (re-)use the post process mapping file.

Would that make sense and perhaps simplify things?

@zelosleone zelosleone marked this pull request as draft July 4, 2025 12:36
Comment on lines +150 to +161
#[cfg(windows)]
{
// Assume this means windows
// On Windows, set_readonly(false) is the correct way to make a file writable
// It sets the FILE_ATTRIBUTE_READONLY attribute to false
#[allow(clippy::permissions_set_readonly_false)]
{
perms.set_readonly(false);
}
}
#[cfg(not(any(unix, windows)))]
{
// Fallback for other platforms
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this part is simply to satisfy clippy

@@ -44,6 +44,7 @@ pub const WIN_ALLOWLIST: &[&str] = &[
"SHELL32.dll",
"USER32.dll",
"USERENV.dll",
"VCRUNTIME140.dll",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on vs2022 build tools we need to add this as well

Comment on lines +80 to +100
compilers = "1.6.0.*"
clang = ">=18.1.8,<19.0"
mold = ">=2.33.0,<3.0"
patchelf = ">=0.17.2,<0.18"

[target.osx-64.dependencies]
compilers = "1.6.0.*"
patchelf = ">=0.18.0,<0.19"

[target.osx-arm64.dependencies]
compilers = "1.6.0.*"
patchelf = ">=0.18.0,<0.19"

[target.win-64.dependencies]
vs2022_win-64 = ">=19.44.35207"

[target.linux-64.activation]
scripts = ["scripts/activate.sh"]
[target.osx-arm64.activation]
scripts = ["scripts/activate.sh"]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i seperated the compilers (that uses vs2019) from windows (manually set it to vs2022 due to bugs i encountered with earlier versions, both in CI and local)

@zelosleone zelosleone marked this pull request as ready for review July 14, 2025 14:29
@zelosleone zelosleone requested a review from wolfv July 14, 2025 14:29
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.

Dynamic linking checks don't work with multiple output cache
2 participants