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

Fix: Fixed SourceGenerator build failure #12471

Merged
merged 7 commits into from
May 28, 2023
Merged

Fix: Fixed SourceGenerator build failure #12471

merged 7 commits into from
May 28, 2023

Conversation

Poker-sang
Copy link
Contributor

Resolved / Related Issues

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Open app ...
    2. Click settings button ...

Screenshots
image

@0x5bfa
Copy link
Member

0x5bfa commented May 28, 2023

We do not use Any CPU.

  • Have to specify respective configurations in following file(Any CPU to x86, x64, arm64):

    Files/Files.sln

    Lines 377 to 406 in 9fab836

    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|arm64.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|arm64.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|x64.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|x64.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|x86.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|x86.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|arm64.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|arm64.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|x64.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|x64.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|x86.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|x86.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|arm64.ActiveCfg = Release|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|arm64.Build.0 = Release|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|x64.ActiveCfg = Release|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|x64.Build.0 = Release|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|x86.ActiveCfg = Release|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|x86.Build.0 = Release|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|arm64.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|arm64.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|x64.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|x64.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|x86.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|x86.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|arm64.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|arm64.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|x64.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|x64.Build.0 = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|x86.ActiveCfg = Debug|Any CPU
    {6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|x86.Build.0 = Debug|Any CPU

  • Have to make have exactly the same property as right one.

    <LangVersion>latest</LangVersion>
    <Nullable>enable</Nullable>
    <IsTrimmable>true</IsTrimmable>
    <Configurations>Debug;Release;Stable;Preview;Store</Configurations>
    <Platforms>x86;x64;arm64</Platforms>
    <RuntimeIdentifiers>win10-x86;win10-x64;win10-arm64</RuntimeIdentifiers>
    • Files.App.Storage

      <TargetFramework>net7.0-windows10.0.22621.0</TargetFramework>
      <TargetPlatformMinVersion>10.0.19041.0</TargetPlatformMinVersion>
      <Nullable>enable</Nullable>
      <IsTrimmable>true</IsTrimmable>
      <Configurations>Debug;Release;Stable;Preview;Store</Configurations>
      <Platforms>x86;x64;arm64</Platforms>
      <RuntimeIdentifiers>win10-x86;win10-x64;win10-arm64</RuntimeIdentifiers>

    • SourceGenerator

      <TargetFramework>netstandard2.0</TargetFramework>
      <LangVersion>preview</LangVersion>
      <Nullable>enable</Nullable>
      <IncludeBuildOutput>false</IncludeBuildOutput>
      <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>

  • May have to remove properties(OutputItemType="Analyzer" ReferenceOutputAssembly="false" )

    <ProjectReference Include="..\Files.Sdk.Storage\Files.Sdk.Storage.csproj" />
    <ProjectReference Include="..\Files.Shared\Files.Shared.csproj" />
    <ProjectReference Include="..\Files.SourceGenerator\Files.SourceGenerator.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />
    </ItemGroup>

@Poker-sang
Copy link
Contributor Author

Poker-sang commented May 28, 2023

@0x5bfa I am sorry. I understand what you mean, but these options must be specified. It is necessary for a Source Generator(analyzer) project. According to MSDN.

OutputItemType="Analyzer" ReferenceOutputAssembly="false"

<IncludeBuildOutput>false</IncludeBuildOutput> 
<EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules> 

And IsTrimable option is available only in .NET6 or greater can be used(MSDN). This project is using .netstandard2.0. In fact, the code of this project is not compiled into the application (because it is an analyzer), so it does not matter if it is trimmed or not

As for the AnyCPU config. I don't mean to use it. But if I use x86, x64 or arm64, it will build failed with CSC: warning CS8034.

@0x5bfa
Copy link
Member

0x5bfa commented May 28, 2023

Thank you for telling those.

In any way, can you remove changes from PathBreadcrumb.xaml.cs for a while? Unfortunately, adding Any CPU is not fundamental issue resolving.

[DependencyProperty<ToolbarViewModel>("ViewModel")]
public sealed partial class PathBreadcrumb : UserControl
{
public PathBreadcrumb()
{
InitializeComponent();
}

@Poker-sang
Copy link
Contributor Author

Thank you for telling those.

In any way, can you remove changes from PathBreadCrumb.xaml.cs? That is the issue now.

I can do it. But I'm concerned that someone has already developed based on the modified PathBreadCrumb.xaml.cs, which could lead to conflicts if I remove it.

@0x5bfa
Copy link
Member

0x5bfa commented May 28, 2023

No worries. The reason why I decided to use this file for a test is this file has much less commit history, as you can see it, there is no functional update in the past months.

You can

  • remove [DependencyProperty<ToolbarViewModel>("ViewModel")]
  • add
    public ToolbarViewModel ViewModel
    {
    	get => (ToolbarViewModel)GetValue(ViewModelProperty);
    	set => SetValue(ViewModelProperty, value);
    }
    
    public static readonly DependencyProperty ViewModelProperty =
    	DependencyProperty.Register(nameof(ViewModel), typeof(ToolbarViewModel), typeof(PathBreadcrumb), new PropertyMetadata(null));

@Poker-sang
Copy link
Contributor Author

Poker-sang commented May 28, 2023

Unfortunately, adding Any CPU is not fundamental issue resolving.

hmm, actually we have already add AnyCPU config, this PR does not add a new config. And ms recommends to use AnyCPU in an analyzer project.

No worries

Ok I will remove it.

Files/Files.sln

Lines 377 to 406 in 40a0f0f

{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|arm64.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|arm64.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|x64.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|x64.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|x86.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Debug|x86.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|arm64.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|arm64.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|x64.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|x64.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|x86.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Preview|x86.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|arm64.ActiveCfg = Release|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|arm64.Build.0 = Release|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|x64.ActiveCfg = Release|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|x64.Build.0 = Release|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|x86.ActiveCfg = Release|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Release|x86.Build.0 = Release|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|arm64.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|arm64.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|x64.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|x64.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|x86.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Stable|x86.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|arm64.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|arm64.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|x64.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|x64.Build.0 = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|x86.ActiveCfg = Debug|Any CPU
{6FA07816-DE0A-4D49-84E8-38E953A33C87}.Store|x86.Build.0 = Debug|Any CPU

@0x5bfa
Copy link
Member

0x5bfa commented May 28, 2023

Oh really? Then keep it please sorry.

And ms recommends to use AnyCPU in an analyzer project.

Can you link?

@Poker-sang
Copy link
Contributor Author

Poker-sang commented May 28, 2023

Can you link?

Sorry, I am not sure if it is mentioned in MSDN. But if you look at any repos, they all use AnyCPU. Such as Roslyn or CommunityToolkit.

@0x5bfa
Copy link
Member

0x5bfa commented May 28, 2023

@Poker-sang I am totally sorry for the inconvenience! Can you keep it(Revert the latest commit)?

@0x5bfa
Copy link
Member

0x5bfa commented May 28, 2023

@yaira2 '@Poker-sang' found the way to make the compiler successful instead of discarding all changes.

@0x5bfa 0x5bfa self-requested a review May 28, 2023 14:09
@Poker-sang
Copy link
Contributor Author

No problem

@Poker-sang
Copy link
Contributor Author

Poker-sang commented May 28, 2023

I only tested GitHub Action and it was successful, I don't know if Azure Pipeline will be fine, hopefully no issues.

@yaira2 yaira2 changed the title Fix: SourceGenerator build failure Fix: Fixed SourceGenerator build failure May 28, 2023
Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!🎉

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 28, 2023
@yaira2
Copy link
Member

yaira2 commented May 28, 2023

@Poker-sang thank you for the quick fix!

@0x5bfa
Copy link
Member

0x5bfa commented May 28, 2023

@yaira2

@yaira2 yaira2 merged commit e8ab068 into files-community:main May 28, 2023
@Poker-sang
Copy link
Contributor Author

Poker-sang commented May 30, 2023

Oh really? Then keep it please sorry.

Can you link?

I think I know why it is configured with AnyCPU. As I said it is an analyzer project and will not be compiled into the application. The DLL it generates will only be loaded by the VS or dotnet compiler. So the target platform of the DLL depends on the CPU architecture of the computer where the code is compiled and not on the user's computer. For example, my computer should be configured with x64, while Azure server might use Arm64. For convenience and without specifying a particular configuration, AnyCPU is the best. ☺️

That's why x86 and Arm64 were wrong before. Because they used x86 or Arm64 configurations, while Azure and Github servers are x64

@0x5bfa
Copy link
Member

0x5bfa commented May 30, 2023

Oh thank you for the long response, and I finally fully understand the reason why. I will add that comment to the project file to mention that.

@0x5bfa
Copy link
Member

0x5bfa commented Jul 7, 2023

@Poker-sang Do you think we can change the attribution to following way?

[DependencyProperty]
public ToolbarViewModel ViewModel { get; set; }

Inspiration:

image

@Poker-sang
Copy link
Contributor Author

Poker-sang commented Jul 7, 2023

Do you think we can change the attribution to following way?

Sure. It seems a way to have better readability. And we can even put comments on it.

@0x5bfa
Copy link
Member

0x5bfa commented Jul 7, 2023

It's nice to hear. Would you like to work on?

@Poker-sang
Copy link
Contributor Author

Maybe we can open an issue first. And I will work on it in a few days. Are you in a hurry to need this?

@0x5bfa
Copy link
Member

0x5bfa commented Jul 7, 2023

Not at all, take your time please. Issued #12889.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants