-
Notifications
You must be signed in to change notification settings - Fork 83
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
base: main
Are you sure you want to change the base?
Conversation
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 |
@@ -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()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
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 Then, every process using the cache contents could also (re-)use the post process mapping file. Would that make sense and perhaps simplify things? |
…sed before too github runners suck)
#[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 |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
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"] | ||
|
There was a problem hiding this comment.
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)
closes #1684
now we will:
since library mapping is also improved, now we will be able to track files provided by each package as well!