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

Fixed data from other samples being pulled into single-experiment solutions #130

Merged
merged 5 commits into from
Jun 6, 2022

Conversation

Arlodotexe
Copy link
Member

Closes #120

Summary

This PR corrects an issue where, in a single-sample solution, data from other samples could appear.

The cause

This was caused by the use of absolute paths + wildcards, which picked up all samples with no regard for the current running experiment.

The solution

Instead of using absolute paths for including source and markdown in single-experiment heads, we're using relative paths exclusively.

This has the side effect of changing where content placed in the application package when running in "All" vs "Single" mode, which is compensated for manually when building the path at runtime.

This PR has been tested against the CanvasLayout solution, Generated "All" solution, and ProjectTemplate solution. UWP, WebAssembly and WinAppSDK were all tested.

@michael-hawker
Copy link
Member

@Arlodotexe looks like some build issues. There's an overload to Split which is valid for .NET 5+ or something but not .NET Standard two, so VS shows you its good, but it's not... I had this happen to me once. I think you just have to create the explicit array to pass in or whatever to get the correct overload that's valid everywhere.

@Arlodotexe
Copy link
Member Author

@Arlodotexe looks like some build issues. There's an overload to Split which is valid for .NET 5+ or something but not .NET Standard two, so VS shows you its good, but it's not... I had this happen to me once. I think you just have to create the explicit array to pass in or whatever to get the correct overload that's valid everywhere.

Thank goodness for CI! I'll get that fixed up :)

@michael-hawker
Copy link
Member

Weird, I've seen this error locally (usually re-building/deploying fixes it), but never in the CI...

"D:\a\Labs-Windows\Labs-Windows\template\lab\ProjectTemplate.sln" (default target) (1:2) ->
"D:\a\Labs-Windows\Labs-Windows\template\lab\samples\ProjectTemplate.Wasm\ProjectTemplate.Wasm.csproj" (default target) (4:6) ->
(BuildDist target) -> 
  C:\Users\runneradmin\.nuget\packages\uno.wasm.bootstrap\3.1.2\build\Uno.Wasm.Bootstrap.targets(175,5): error : Failed to copy D:\a\Labs-Windows\Labs-Windows\common\..\ProjectTemplate.Sample\ProjectTemplate.md to D:\a\Labs-Windows\Labs-Windows\template\lab\samples\ProjectTemplate.Wasm\obj\Debug\net5.0\dist_work\SourceAssets\ProjectTemplate.Sample\ProjectTemplate.md: Could not find a part of the path 'D:\a\Labs-Windows\Labs-Windows\common\..\ProjectTemplate.Sample\ProjectTemplate.md'. [D:\a\Labs-Windows\Labs-Windows\template\lab\samples\ProjectTemplate.Wasm\ProjectTemplate.Wasm.csproj]

I've re-run that job and we'll see if it happens again. But if that can randomly fail in the CI, that'll be a bit concerning. Imagine we'd want to open an Uno/.NET issue or something for that? @Arlodotexe?

@Arlodotexe
Copy link
Member Author

Arlodotexe commented May 23, 2022

Failed to copy D:\a\Labs-Windows\Labs-Windows\common\..\ProjectTemplate.Sample\ProjectTemplate.md

That isn't a valid path....

This might be related to the changes in this PR. Will investigate

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned we're just digging a whole deeper here. Can we step-back and evaluate if we can change our approach or look for alternatives to simplify this?

I mean part of this is a workaround for Uno with #37, #88, and #90. Part of this is the structure of our projects.

Can we document where each type of source file we need access to lives in the repo structure from the root and where it's looked for in each of the scenarios (all samples, single experiment, template)? And then decide a single desired path we want those to be in?

As ideally the sample app just gets the same path to SourceAssets\ExperimentName\csorxamlordocfilename regardless of what mode the app is running in or how it was built... that'd solve a lot of issues now and in the future, right?

common/Labs.Head.props Outdated Show resolved Hide resolved
common/Labs.Head.props Outdated Show resolved Hide resolved
@michael-hawker
Copy link
Member

Hey @jeromelaban, we're getting bit again more by the unoplatform/uno#2502 (comment) as we're running into issues trying to wildcard this globally vs. just being able to grab things from the assemblies as we're building things in multiple modes/locations depending on the scenario. It bleeds into our source/generator/app logic now to try and figure out where our assets area going to be.

Any update or other suggestions? Thanks!

@michael-hawker
Copy link
Member

@Arlodotexe talked to Jerome. He suggested instead of the glob we could use an inline task to help us with our desired copying heuristic: https://github.com/unoplatform/uno/blob/43efd99f67daade2e663e317989c3ab324a2c443/src/SolutionTemplate/Uno.ProjectTemplates.Dotnet/Uno.ProjectTemplates.Dotnet.csproj#L244

That way we can use the proper globs to grab the input files from the proper locations, but then have the task make sure the extra parent/relative path parts don't matter as it'll copy it to a consistent SourceAssets\ExperimentName.Sample\files location which matches the metadata we generate. That way all the apps should be able to load consistently regardless of where we're building the app / grabbing the files from, right?

@michael-hawker
Copy link
Member

@Arlodotexe I opened CommunityToolkit/Tooling-Windows-Submodule#18 to track improving the workaround later (if we need to if the Uno issue remains unfixed). Can we just add comments in the cs/csproj to point to our Labs issue and then we can merge this in?

@michael-hawker michael-hawker merged commit 5203527 into main Jun 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/multiple-tabs-in-single-sample branch June 6, 2022 22:09
Martin1994 pushed a commit to Martin1994/Labs-Windows that referenced this pull request Sep 2, 2023
…tiple-tabs-in-single-sample

Fixed data from other samples being pulled into single-experiment solutions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent number of tabs in sample apps
3 participants