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.

Sign up for GitHub

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

[build] Fix ordering issues with ExposeExperimentalFeatures compiler switch #4828

Merged
merged 3 commits into from
Sep 6, 2023
Merged

[build] Fix ordering issues with ExposeExperimentalFeatures compiler switch #4828

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
869a538
Fix ExposeExperimentalFeatures not updating after MinVer resolution.
CodeBlanch Sep 6, 2023
e535e5a
Fix integration test builds.
CodeBlanch Sep 6, 2023
c3b4a60
Cleanup.
CodeBlanch Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions build/Common.prod.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,19 @@
<Message Importance="high" Text="**AssemblyVersionDebug** TargetFramework: $(TargetFramework), MinVerVersion: $(MinVerVersion), MinVerMajor: $(MinVerMajor), MinVerMinor: $(MinVerMinor), MinVerPatch: $(MinVerPatch), MinVerPreRelease: $(MinVerPreRelease), BuildNumber: $(BuildNumber), FileVersion: $(FileVersion), ExposeExperimentalFeatures: $(ExposeExperimentalFeatures)" />
</Target>

<Target Name="ResolveExposeExperimentalFeatures" AfterTargets="MinVer">
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be run after AssemblyVersionTarget?

Suggested change
<Target Name="ResolveExposeExperimentalFeatures" AfterTargets="MinVer">
<Target Name="ResolveExposeExperimentalFeatures" AfterTargets="AssemblyVersionTarget">

Copy link
Member Author

Choose a reason for hiding this comment

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

I had that at first but AssemblyVersionTarget gets skipped for non-publish builds and we want this section to always run.

Copy link
Contributor

Choose a reason for hiding this comment

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

When publishing builds, is the ordering guaranteed? We don't want this target to be evaluated before AssemblyVersionTarget.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not 100% sure about that, but I changed it so it now has a DependsOnTargets="AssemblyVersionTarget"

<PropertyGroup Condition="'$(ExposeExperimentalFeatures)' == 'true'">
<DefineConstants>$(DefineConstants);EXPOSE_EXPERIMENTAL_FEATURES</DefineConstants>
</PropertyGroup>

<!--PublicApi Analyzer-->
<ItemGroup>
<AdditionalFiles Include=".publicApi\Stable\$(TargetFramework)\PublicAPI.*.txt" />
<AdditionalFiles Include=".publicApi\Experimental\$(TargetFramework)\PublicAPI.*.txt" Condition="'$(ExposeExperimentalFeatures)' == 'true'" />
<AdditionalFiles Include=".publicApi\$(TargetFramework)\PublicAPI.*.txt" />
</ItemGroup>
</Target>

<PropertyGroup>
<RepositoryType>git</RepositoryType>
<RepositoryUrl>https://github.com/open-telemetry/opentelemetry-dotnet</RepositoryUrl>
Expand Down Expand Up @@ -92,9 +105,6 @@

<!--PublicApi Analyzer-->
<ItemGroup>
<AdditionalFiles Include=".publicApi\Stable\$(TargetFramework)\PublicAPI.*.txt" />
<AdditionalFiles Include=".publicApi\Experimental\$(TargetFramework)\PublicAPI.*.txt" Condition="'$(ExposeExperimentalFeatures)' == 'true'" />
<AdditionalFiles Include=".publicApi\$(TargetFramework)\PublicAPI.*.txt" />
<None Include=".publicApi\**\PublicAPI.*.txt" />
</ItemGroup>

Expand Down
4 changes: 0 additions & 4 deletions build/Common.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@
<ExposeExperimentalFeatures Condition="'$(ExposeExperimentalFeatures)' == ''">true</ExposeExperimentalFeatures>
</PropertyGroup>

<PropertyGroup Condition="'$(ExposeExperimentalFeatures)' == 'true'">
<DefineConstants>$(DefineConstants);EXPOSE_EXPERIMENTAL_FEATURES</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition="'$(Configuration)'=='Debug'">
<DebugType>full</DebugType>
<DebugSymbols>true</DebugSymbols>
Expand Down