MRTCore will also search for [modulename].pri for unpackaged app #814
Conversation
ghost
added
the
It would be helpful for the reader if you sprinkled comments in the body. For example, a comment for this while block will help. In reply to: 837708365 In reply to: 837708365 Refers to: dev/MRTCore/mrt/Core/src/MRM.cpp:878 in 18d0d84. [](commit_id = 18d0d84, deletion_comment = False) |
| MrmFreeResource(path); | ||
|
|
||
| Assert::AreEqual(MrmGetFilePathFromName(L"something.pri", &path), S_OK); | ||
| // Even if the file doesn't exist, we will still return a path. This is for those don't use PRI for resource. |
There was a problem hiding this comment.
an existing app may want to migrate to switch to WinUI3, but not ready to switch the resource management yet. They can use ResourceNotFound event to hook up their existing resource management.
There was a problem hiding this comment.
Adding that or part of that as a comment would be good, IMO.
There was a problem hiding this comment.
Agreed. The comment on 963-964 of MRM.cpp was not enlightening because it didn't explain why that was the desired behavior.
dev/MRTCore/mrt/Microsoft.ApplicationModel.Resources/UnpackagedTests/TestClass.cs
Show resolved
Hide resolved
|
One question: in the wapproj case, will resources.pri exist alongside <csproj/vcxproj name>.pri? (resources.pri is a merge of <csproj/vcxproj name>.pri and other stuff) Assuming it does, if there is an issue generating resources.pri, our code will now succeed (it'll fall back to <csproj/vcxproj name>.pri). Don't we want it to fail instead? Otherwise the change looks good to me. |
In reply to: 840038734 |
|
Regarding #2 above, with wapproj-less packaged apps, we do want to use .pri, don't we? There won't be a resources.pri in that case, no? EDIT: looking at the code, it seems 2 is not true. GetDefaultPriFile() is only ever called with an empty path. So, if we fall back to the second method (in the impl of GetDefaultPriFile()), we will look for .pri (if we can't find resources.pri). I agree that 1 is true. This one edge case comes to mind: is it possible to do a clean of the wapproj only where resources.pri is removed but .pri remains? In reply to: 840038734 |
| @@ -145,6 +145,7 @@ steps: | |||
| !**\*TestAdapter.dll | |||
| !**\TE.*.dll | |||
| !**\obj\** | |||
| !**\MrtCoreUnpackagedTests.dll | |||
| { | ||
| *filePath = nullptr; | ||
|
|
||
| RETURN_HR_IF(E_INVALIDARG, filename == nullptr || *filename == L'\0'); | ||
|
|
||
| wchar_t path[MAX_PATH]; |
There was a problem hiding this comment.
Maybe not as part of this PR, but we should remove usages of MAX_PATH.
|
for any packaged app (wap or not), the resource file needs to be resources.pri. If it's not, we need to make that happen, or the app may have issue with system MRT. In reply to: 840116910 |
| MrmFreeResource(path); | ||
|
|
||
| Assert::AreEqual(MrmGetFilePathFromName(L"something.pri", &path), S_OK); | ||
| // Even if the file doesn't exist, we will still return a path. This is for those don't use PRI for resource. |
There was a problem hiding this comment.
Agreed. The comment on 963-964 of MRM.cpp was not enlightening because it didn't explain why that was the desired behavior.
For unpackaged app, the tool will build the resource file as [modulename].pri. It's not straightforward to update build tool. Change the runtime behavior to also look up [modulename].pri for unpackaged app.
MrtCoreUnpackagedTest only runs in command line, as the test adapter launches te.processhost.exe from nuget cache folder, thus we cannot copy manifest and pri file there.