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

[postbuildlint] Don't read lib info twice #1384

Merged
merged 10 commits into from Apr 16, 2024

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Apr 8, 2024

Library information was read twice from the same files: once in check_lib_architecture and once in check_crt_linkage_of_libs. This caused a performance regression. This PR moves reading the libraries to a single function (get_lib_info) which is only called once.

Additionally, check_lib_architecture performed the check cmake_system_name == "Darwin" but this function is only called on Windows. Therefore, this check is unnecessary and I removed the code inside it.

@Thomas1664
Copy link
Contributor Author

Additionally, check_lib_architecture performed the check cmake_system_name == "Darwin" but this function is only called on Windows. Therefore, this check is unnecessary and I removed the code inside it.

@BillyONeal I just realised that you messed this up in #834. Considering that fixing this will most likely need verification with microsoft/vcpkg and e2e tests, how to proceed with this?

@BillyONeal
Copy link
Member

@BillyONeal I just realised that you messed this up in #834. Considering that fixing this will most likely need verification with microsoft/vcpkg and e2e tests, how to proceed with this?

I don't think we've ever seen this break before so I'd be happy to take your fix without new tests. (I would love new tests, but I'm not enough of a macOS person to write them myself and I don't think 'restoring what should have been the status quo' qualifies as the kind of 'new feature' ask for which we require tests.)

auto release_lib_info = get_lib_info(fs, release_libs);
Optional<std::vector<Optional<LibInformation>>> debug_lib_info;

if (!build_info.policies.is_enabled(BuildPolicy::SKIP_ARCHITECTURE_CHECK) ||
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of adding conditionals that need to be paired like this or the world ends but I don't have better ideas for now.

@BillyONeal BillyONeal enabled auto-merge (squash) April 16, 2024 19:19
@BillyONeal BillyONeal merged commit 1603ecb into microsoft:main Apr 16, 2024
5 checks passed
@Thomas1664
Copy link
Contributor Author

Do you want me to restore the lib check on OSX in another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants