Skip to content

Conversation

@andrew-polk
Copy link
Contributor

@andrew-polk andrew-polk commented Dec 8, 2025

This change is Reviewable

@github-actions
Copy link

github-actions bot commented Dec 8, 2025

Palaso Tests

     4 files       4 suites   9m 57s ⏱️
 5 070 tests  4 836 ✅ 234 💤 0 ❌
16 525 runs  15 804 ✅ 721 💤 0 ❌

Results for commit 9e09c6f.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@tombogle tombogle left a comment

Choose a reason for hiding this comment

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

@tombogle reviewed 2 of 8 files at r1, all commit messages.
Reviewable status: 2 of 8 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)


SIL.Core.Desktop/SIL.Core.Desktop.csproj line 11 at r1 (raw file):

    <TargetFrameworks>$(TargetFrameworks);netstandard2.0</TargetFrameworks>
  </PropertyGroup>
  <PropertyGroup Condition=" '$(TargetFramework)' == 'net462' OR '$(TargetFramework)' == 'net48' ">

ChatGPT seems to think this would work, so we wouldn't have to hard-code specific versions. If not, you could try this instead: Condition="$(TargetFramework.StartsWith('net4'))"

Code snippet:

<PropertyGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
  <DefineConstants>$(DefineConstants);NETFRAMEWORK</DefineConstants>
</PropertyGroup>

@andrew-polk
Copy link
Contributor Author

SIL.Core.Desktop/SIL.Core.Desktop.csproj line 11 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

ChatGPT seems to think this would work, so we wouldn't have to hard-code specific versions. If not, you could try this instead: Condition="$(TargetFramework.StartsWith('net4'))"

The only Condition which works for me in all projects all the time is the explicit check for net462 or net48.

The two you suggest do not.
Neither does
Condition\="'$(IsNetFramework)' == 'true'"

Sonnet claims it is because you can't rely on these variables being set at the time of evaluation. Not sure I undestand or buy that. But I do know that experimentally, all the other options fail at least sometimes at in some projects.

Copy link
Contributor

@tombogle tombogle left a comment

Choose a reason for hiding this comment

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

@tombogle reviewed 6 of 8 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @andrew-polk)

@andrew-polk andrew-polk merged commit ae035eb into master Dec 8, 2025
11 checks passed
@andrew-polk andrew-polk deleted the FixConstant branch December 8, 2025 22:01
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.

3 participants