MRTCore will also lookup parent folder #554

huichen123 · 2021-03-11T00:52:38Z

When file path is not provided (default or only filename), MRTCore will also lookup current module path as well as the parent folder for the resource file. This is to accommodate Visual Studio which usually creates EXE in a subfolder with project name unless the project has TargetPlatformIdentifier set to UAP.

rohanp-msft · 2021-03-11T01:04:07Z

dev/MRTCore/mrt/Core/src/MRM.cpp


-    *filePath = outputPath.release();
+    *filePath = finalPath.release();

    return S_OK;


Could we have the fallback impl be in a separate function that we call if filePath is empty after the existing code runs?

not quite sure what you mean. but this has least code churn.

The existing code looks in the current module path, right? We're adding looking in the parent folder too. I'm saying having the addition, i.e., looking in the parent folder, be in a separate function. That way if we want to remove it in the future, for example, it is much easier. It also makes the code more self-explanatory.

In reply to: 591992825 [](ancestors = 591992825)

then the second function will be similar to the first one. People will ask why you cannot abstract the logic. Plus, a few more places need to be updated. Similar logic of lookup/check existence/lookup again will also need to apply to multiple places. If you want to prepare for possible future removal, this is probably the easiest.

ok, this is fine with me

In reply to: 591996471 [](ancestors = 591996471)

rohanp-msft · 2021-03-11T01:15:32Z

dev/MRTCore/mrt/Core/src/MRM.cpp

-    RETURN_IF_NULL_ALLOC(rawOutputPath);
+        if (i == 0)
+        {
+            // If none of the file exists, will return the file in current path


will return the file in current path [](start = 43, length = 36)

But if the file exists in the current path, we won't even get here. We will break above.

this is when the file doesn't exist in either location, we will return the file path in current folder.

but if we're here, the file doesn't exist in the current folder, no?

In reply to: 591996835 [](ancestors = 591996835)

correct. this function returns a filename. If the file doesn't exist, it will still return you a name. There is no point to fail.

rohanp-msft

riverar

That path manipulation code is a bit scary at glance, would like to see another PR at some point replace it with std::experimental::filesystem::path. But sounds like minimal churn was your goal for now.

riverar · 2021-03-11T01:21:34Z

dev/MRTCore/mrt/Core/src/MRM.cpp

+        RETURN_IF_FAILED(StringCchCopyW(outputPath.get(), length, pointerToPath));
+        RETURN_IF_FAILED(StringCchCatW(outputPath.get(), length, filename));
+
+        if (GetFileAttributes(outputPath.get()) != INVALID_FILE_ATTRIBUTES)


This will also succeed if a directory, not a file, with the same name exists. Might want to explicitly test for and exclude items with FILE_ATTRIBUTE_DIRECTORY.

Yes. There is little time before release. Also this is no throwing code, and this doesn't use std.
I was debating whether exclude directory or make code simple. The chance you have a folder called resources.pri is extremely slim. I can make that check if you insist.

Ha! I'm an external so can't insist anything. Agree the risk is low, we can create an issue to track it and move forward.

Note from Will: May not actually include all these changes in the same form anymore. * Update framework udk (#511) * Cherry pick VSIX into 0.5 preview release branch (#517) * Update roadmap (unpackaged apps to 0.8) (#506) * Update roadmap (unpackaged apps to 0.8) * Include a subscribe option * Port VS changes in Microsoft.AppxPackage.targets to Microsoft.ApplicationModel.Resources.PriGen.targets: reduce messages printed to output to improve performance. (#485) * Initial checkin of Reunion VSIX, ported from WinUI3 (#496) * Initial checkin of Reunion VSIX, ported from WinUI VSIX sources * update nupkgs and fix wap appx references * added missing pubxml files * path bug, reunion tags * cruft, pubxml * remove Microsoft.WinUI.AppX.targets * remove asset dupes * PR feedback on appx manifests * replace nuget.config with RestoreSources to support pipeline, fix bad vstemplate * updated readmes * Initial checkin of Reunion VSIX, ported from WinUI3 (#496) * Initial checkin of Reunion VSIX, ported from WinUI VSIX sources * update nupkgs and fix wap appx references * added missing pubxml files * path bug, reunion tags * cruft, pubxml * remove Microsoft.WinUI.AppX.targets * remove asset dupes * PR feedback on appx manifests * replace nuget.config with RestoreSources to support pipeline, fix bad vstemplate * updated readmes Co-authored-by: Andrew Leader <aleader@microsoft.com> Co-authored-by: Rohan Palaniappan <35987549+rohanp-msft@users.noreply.github.com> * Tweaks for Reunion 0.5 Preview release (#524) (#525) * candidate versions of 0.5 preview nupkgs * update to latest nupkgs * another update and template fix * Cherry pick commit 8f2e841: Revert naming of win32 apps' PRI to the project name. (#529) * address build/restore issues causing vsix pipeline failures (#530) * Cherry pick: create VSIX pipeline (#509) (#537) * User/ertorres/create vsix (#509) * remove nuget org feed * remove vssdk overhead * always use internal feed (#547) * Search parent folder (#554) (#569) * simplify wap project templates to enable PackageReferences upgrades, ensure that ad hoc PackageReferences work for wapproj (#578) * Bump transport package version to 0.5 (#576) (#581) * GA tweaks - VS 16.9 prereq, WinUI 3 rename, remove UWP templates, Suspending event (#591) * GA tweaks - VS 16.9 prereq, WinUI 3 rename, UWP templates unsupported * remove UWP templates - not supported in release * remove experimental Suspending event * Update FrameworkUDK release (#614) * Remove unnecessary Pkg* defines and avoid bad vsix builds when nuget restore fails (still investigating those) (#631) * Duplicate MSBuildProjectExtensionsPath was causing inconsistent nuget restore and broken vsix builds (#640) * fixing regression from earlier VSIX MSbuild change that moved published file (#643) Co-authored-by: Ben Kuhn <benkuhn@ntdev.microsoft.com> * drop preview tag (#646) Co-authored-by: Ben Kuhn <benkuhn@ntdev.microsoft.com> * User/benkuhn/add vsix patch option (#654) * adding VSIX version override support * formatting fix * non-empty default value * updating how versions are handled in build script * trying again * trying again * trying again * debugging * variable reference fix * simpler version Co-authored-by: Ben Kuhn <benkuhn@ntdev.microsoft.com> * Set SDKVersion (#674) (#684) * Move MrtCore up to cswinrt 1.2 to keep in sync with WinUI which is doing likewise to pick up memory leak fixes (#685) * Switch to Open Helix Queues (#665) (#706) Co-authored-by: Keith Mahoney <41657372+kmahone@users.noreply.github.com> * Update MRTCore to .net 5.0.3 (#703) (#705) * Update MRTCore to .net 5.0.3 (#703) * Update MRTCore to .net 5.0.3 * Update runtime version * Change task display name * update to cswinrt 1.2.1 and .NET 5.0.5 (#715) * update to cswinrt 1.2.1 and .NET 5.0.5 * syncing with latest dotnet update changes * revert framework udk versions * update SDK.NET.Ref to 16 * update CsWinRT to 1.2.2 Co-authored-by: Scott Jones <sjones@microsoft.com> Co-authored-by: Andrew Leader <aleader@microsoft.com> Co-authored-by: Rohan Palaniappan <35987549+rohanp-msft@users.noreply.github.com> Co-authored-by: Erik Torres <erik.torres.aguilar@gmail.com> Co-authored-by: Hui Chen <huichen@microsoft.com> Co-authored-by: Ben Kuhn <bjk4929@yahoo.com> Co-authored-by: Ben Kuhn <benkuhn@ntdev.microsoft.com> Co-authored-by: Keith Mahoney <41657372+kmahone@users.noreply.github.com>

Search parent folder

5f31e60

huichen123 requested review from rohanp-msft and axelandrejs March 11, 2021 00:52

ghost added the needs-triage label Mar 11, 2021

axelandrejs approved these changes Mar 11, 2021

View reviewed changes

rohanp-msft reviewed Mar 11, 2021

View reviewed changes

rohanp-msft approved these changes Mar 11, 2021

View reviewed changes

Merge branch 'main' into user/huichen/checkparentpath

71c6e07

riverar reviewed Mar 11, 2021

View reviewed changes

huichen123 merged commit d177c8b into main Mar 11, 2021

huichen123 deleted the user/huichen/checkparentpath branch March 11, 2021 02:09

huichen123 added a commit that referenced this pull request Mar 11, 2021

Search parent folder (#554)

3d376d1

huichen123 mentioned this pull request Mar 11, 2021

Cherry pick: MRTCore will also lookup parent folder #569

Merged

huichen123 added a commit that referenced this pull request Mar 12, 2021

Search parent folder (#554) (#569)

d57f323

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MRTCore will also lookup parent folder #554

MRTCore will also lookup parent folder #554

huichen123 commented Mar 11, 2021

rohanp-msft Mar 11, 2021

huichen123 Mar 11, 2021

rohanp-msft Mar 11, 2021

huichen123 Mar 11, 2021

rohanp-msft Mar 11, 2021

rohanp-msft Mar 11, 2021

huichen123 Mar 11, 2021

rohanp-msft Mar 11, 2021

huichen123 Mar 11, 2021

rohanp-msft left a comment

riverar left a comment •

edited

Loading

riverar Mar 11, 2021

huichen123 Mar 11, 2021

riverar Mar 11, 2021

MRTCore will also lookup parent folder #554

MRTCore will also lookup parent folder #554

Conversation

huichen123 commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohanp-msft left a comment

Choose a reason for hiding this comment

riverar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riverar left a comment •

edited

Loading