Skip to content

Conversation

@HavenDV
Copy link
Member

@HavenDV HavenDV commented Aug 8, 2022

GitHub Issue (If applicable): closes #
According this discussion: #9145
The first PR for this, while fixing the name collision, didn't fix the availability of the constants and led to new research.

PR Type

What kind of change does this PR introduce?

Feature

What is the current behavior?

At the moment, constants are added after the .editorconfig file with visible properties is generated, which generators use to access values. Here is the relevant code that shows which Targets are executed and in what order to allow generators to access properties.
https://github.com/dotnet/roslyn/blob/1c3cdb0b85a8496a8f5108faab046924360b6017/src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets#L155-L196
Based on this, I believe that the optimal place to add constants would be AfterTargets="PrepareForBuild". This also applies to other projects - https://github.com/dotnet/ef6tools/blob/c9988fc28258290118e60dc6d9ccb3fe8c6073d6/setup/wix/EFToolsWillowMsi.wixproj#L77 Generators that rely on these constants will need to use code like this:

<Target Name="CreateDefineConstants" BeforeTargets="GenerateMSBuildEditorConfigFileShouldRun;GenerateMSBuildEditorConfigFileCore">

    <PropertyGroup>
      <Generator_DefineConstants>$(DefineConstants.Replace(';',','))</Generator_DefineConstants>
    </PropertyGroup>

  </Target>

What is the new behavior?

Targets are run at an earlier stage. In addition, the association of properties has been simplified, and duplication of constants in runtime projects due to the reuse of UnoDefineConstants has been fixed. This output is given by this target in the Skia.WPF project:

  <Target Name="Test" BeforeTargets="CoreCompile">
    <Message Importance="high" Text="Test $(DefineConstants)"/>
  </Target>
1>Test TRACE;HAS_UNO;HAS_WINUI;HAS_UNO_WINUI;RELEASE;NET;NET6_0;NETCOREAPP;UNO_REFERENCE_API;HAS_UNO_SKIA;HAS_UNO_SKIA_WPF;UNO_REFERENCE_API;HAS_UNO_SKIA;HAS_UNO_SKIA_WPF;HAS_UNO;HAS_UNO_WINUI;UNO_HAS_FRAMEWORKELEMENT_MEASUREOVERRIDE;UNO_HAS_NO_IDEPENDENCYOBJECT;UNO_REFERENCE_API

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@gitpod-io
Copy link

gitpod-io bot commented Aug 8, 2022

@jeromelaban jeromelaban merged commit 6ceaab2 into unoplatform:master Aug 15, 2022
@HavenDV
Copy link
Member Author

HavenDV commented Aug 16, 2022

I confirm that the generator is able to see constants since version 4.5.0-dev.523

@HavenDV HavenDV deleted the make-visible-constants-to-generators-2 branch August 16, 2022 04:00
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.

2 participants