Hi Jinoh. Not sure why you requested review... should I remember stuff I wrote 2^4 years ago ?
Sorry. I found very few people who have worked on this. I guess I should just ask for review from julliard directly.
```sh $ git log --pretty='tformat:%an <%ae>' wine-8.6 -- dlls/ntdll/actctx.c | sort | uniq -c | sort -nr | head 69 Alexandre Julliard julliard@winehq.org 38 Nikolay Sivov nsivov@codeweavers.com 14 Eric Pouech eric.pouech@wanadoo.fr 8 Jacek Caban jacek@codeweavers.com 3 Roman Mindalev lists@r000n.net 3 Michael Stefaniuc mstefani@winehq.org 2 Zebediah Figura z.figura12@gmail.com 2 Mikolaj Zalewski mikolajz@google.com 2 Michael Müller michael@fds-team.de 2 Maarten Lankhorst m.b.lankhorst@gmail.com ```
comments mostly concerning last commit:
- picking first ID instead of ID 1 doesn't look fully right, at least independently of context
Sure it might, but it's what Windows does. The `test_valid_manifest_resources` function (from patch 4/5) has tests for arbitrary resource IDs and selection priority.
- from MS doc, , it seems file is valid iff only one of ID 1, 2 or 3 is present (and ID up to 16 are reserved)
Theoretically yes, but from testing it seems that Windows don't validate (or care about) multiple IDs at all. I guess each of them is used by an independent component, as described in https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_29411.
- this should at least be tested against bogus configuration
By configuration do you mean DLL resources, or system configuration?
- so I wonder (I don't pretend to have a definitive insight on this, but it could make sense) if instead of [ vanishing the ID 1 and blindly using first id ], we rather should a) either move up to the callers for the ID resource value, b) or check in callers if the candidate ID value meets the caller requirements
It sounds similar to the original staging patch, https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/ntdll-Mani.... I believe my patchset mimics Windows behavior more closely, as it has tests that back it up.