-
Notifications
You must be signed in to change notification settings - Fork 272
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
[z-applocal] Reduce nonexistent and non-PE files to warnings. #1274
[z-applocal] Reduce nonexistent and non-PE files to warnings. #1274
Conversation
ExpectedL<Unit> read_pe_signature_and_get_coff_header_offset(ReadFilePointer& f) | ||
// reads f as a portable executable and checks for magic number signatures. | ||
// if an I/O error occurs, returns the error; otherwise, | ||
// returns iff signatures match |
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.
// returns iff signatures match | |
// returns if signatures match |
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 was intentional (iff == if and only if)
AppLocalInvocation invocation(real_filesystem, | ||
|
||
// the first binary is special in that it might not be a DLL or might not exist | ||
const Path target_binary_path = target_binary->second; |
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.
const Path target_binary_path = target_binary->second; | |
const Path& target_binary_path = target_binary->second; |
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 think that would be wrong because there is a conversion intended here. (target_binary->second
is a std::string
, not a Path
)
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.
In this case I'd use const Path target_binary_path(target_binary->second)
to make the conversion more obvious.
Checks::exit_fail(VCPKG_LINE_INFO); | ||
} | ||
|
||
auto maybe_dll_metadata = vcpkg::try_read_dll_metadata(dll_file).value_or_exit(VCPKG_LINE_INFO); |
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.
Why not use try_read_dll_metadata_required
?
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.
Because the whole point of this is to use the not required one.
A customer tried to turn on our
z-applocal
built in replacement forapplocal.ps1
and it failed onvcxproj
s that produce no outputs, andvcxproj
s that make static libraries in their testing. This change reduces both results to warnings and explains when there's an MZ signature mismatch better.Before:
After: