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

Exclude global.json from iOS Bundle Resources #8724

Merged
merged 3 commits into from
Aug 1, 2022

Conversation

TanayParikh
Copy link
Contributor

@TanayParikh TanayParikh commented Jul 13, 2022

We were including the global.json as part of the iOS (and Mac Catalyst) bundle resources, which was leading to a build failure. Added it to the BlazorWebView targets filtering logic being used to filter out scoped css files.

Before:
image

After:
image

Note this was an issue specific to iOS and MacCatalyst. Windows/Android scenarios were not impacted.

Fixes: dotnet/aspnetcore#41397

@TanayParikh TanayParikh added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Jul 13, 2022
@TanayParikh TanayParikh self-assigned this Jul 13, 2022
@TanayParikh TanayParikh requested a review from a team as a code owner July 13, 2022 20:54
Comment on lines 70 to 71
<!-- Add global.json to the list of hidden items -->
<_TemporaryHiddenContent Include="global.json" />
Copy link
Contributor Author

@TanayParikh TanayParikh Jul 13, 2022

Choose a reason for hiding this comment

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

Note: Issue also repros with other json files, this isn't unique to global.json. Should we be filtering out all json files from the bundle resources?

      <_TemporaryHiddenContent Include="**/*.json" />

leads to:

image

Copy link
Member

Choose a reason for hiding this comment

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

Note: Issue also repros with other json files, this isn't unique to global.json. Should we be filtering out all json files from the bundle resources?

Seems like in that case it would be easier to just not include them:

https://github.com/dotnet/sdk/blob/6698cd97f38ea3ad2c7182f95e5b9ec887c29f56/src/RazorSdk/Sdk/Sdk.Razor.StaticAssets.ProjectSystem.props#L33

Copy link
Contributor Author

@TanayParikh TanayParikh Jul 14, 2022

Choose a reason for hiding this comment

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

I'd considered that approach as well, however given this issue was iOS specific I wanted to try out a more targeted fix. Though if the JSON files aren't otherwise needed we may be able to do this in the sdk as you suggest. @javiercn thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Not specific to this PR, but rather these exclusions in general: Everything about all of this is hacky, so I have a hard time making any rational statements about this. There's just no way to know what the user truly intended in the general case, so we end up with a heuristic, which means "we know this is wrong, but it's better than the alternative of having nothing."

So, party on 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rolfbjarne do we know why MAUI iOS/MacCatalyst doesn't like these files whilst Android/Windows are fine with them? I fear this may lead to scenarios where apps are working as expected for users on Android/Windows, but silently fail on iOS/MacCatalyst (if we exclude all .json).

Copy link
Member

Choose a reason for hiding this comment

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

@rolfbjarne do we know why MAUI iOS/MacCatalyst doesn't like these files

  1. For iOS and Mac Catalyst any Content files are (supposed to be) added the app bundle.
  2. That requires knowing where to put the files inside the app bundle.
  3. There's a default logic (compute the relative path relative to the project file), and that's where Content files are located inside the app bundle.
  4. When applying this logic to the global.json file this issue is about, the resulting file in the app bundle turns out to be way outside the app bundle (you can see the computed path in the error message).
  5. This is obviously not correct, so we show an error.

It's possible to override what's done about Content files by setting the PublishFolderType metadata (setting PublishFolderType=None would make iOS/Mac Catalyst not try to add the file to the app bundle)

Copy link
Member

Choose a reason for hiding this comment

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

It's possible to override what's done about Content files by setting the PublishFolderType metadata (setting PublishFolderType=None would make iOS/Mac Catalyst not try to add the file to the app bundle)

It might be reasonable if the .NET SDK (or the Razor/web SDK) applied this. I can't see a reason why global.json should be ever published.

<ItemGroup>
  <Content Update="@(Content)" Condition="'%(Content.FileName)' == 'global' and '%(Content.Extension)' == '.json'" PublishFolderType=None />
</ItemGroup>

Should do the trick, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

172cade updates to use PublishFolderType within this target. @javiercn would this be appropriate for now, or do you think it's worthwhile updating the SDK with this exclusion?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine, but it is probably best if it lived in the SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look next time I make SDK related changes. Want to avoid the whole flow just for this fix right now.

@TanayParikh TanayParikh merged commit 0bb81e2 into main Aug 1, 2022
@TanayParikh TanayParikh deleted the taparik/fixAdditionalAssetsInNETMAUIBlazorIOSMac branch August 1, 2022 19:44
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView fixed-in-7.0.0-rc.1.6683
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional files in blazor hybrid project throw error by default
5 participants