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

feat: Targets with constants moved to an earlier stage. #9443

Merged

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.

None yet

2 participants