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

ICU-22977 Fix MSVC builds - Forcing target version #3278

Closed
wants to merge 1 commit into from

Conversation

mihnita
Copy link
Contributor

@mihnita mihnita commented Nov 21, 2024

Defined TargetFrameworkVersion, following this doc:
https://learn.microsoft.com/en-us/cpp/build/how-to-modify-the-target-framework-and-platform-toolset?view=msvc-170#target-framework-ccli-project-only

I am not sure if it is the right fix. It worked just fine for so many years.

Will see what the GitHub team says.

Checklist

  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22977
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@mihnita mihnita marked this pull request as draft November 21, 2024 09:46
@mihnita mihnita marked this pull request as ready for review November 21, 2024 10:27
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@mihnita mihnita changed the title ICU-22977 Fix MSVC builds - Second way ICU-22977 Fix MSVC builds - Forcing target version Nov 21, 2024
@rp9-next
Copy link
Contributor

Thanks for working on this! I too was looking into some solutions such as retargeting platform version (#3277), but couldn't get it to work without reverting to windows-2019 which made me believe this might be another vm runner update of MSVC issue.

May I ask, how did you get to <TargetFrameworkVersion>10.0</TargetFrameworkVersion> ?

Looking at docs, it seems to be the .NET Framework version the latest of which is .NET 4.8 or (.NET 8 for .NET core). Was this set through visual studio IDE ?

@@ -106,7 +107,7 @@
<TurnOffAssemblyGeneration>true</TurnOffAssemblyGeneration>
<IgnoreSpecificDefaultLibraries>vccorlib.lib;msvcrt.lib</IgnoreSpecificDefaultLibraries>
<!-- The icudt.lib is for U_ICUDATA_ENTRY_POINT -->
<AdditionalDependencies>icudt.lib;onecore.lib;%(AdditionalDependencies)</AdditionalDependencies>
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the arm build is failing due to this change ?

@rp9-next
Copy link
Contributor

.NET 8 for .NET core

My bad, .NET 9 was released a few days ago that gets included in Visual Studio 17.12. Perhaps that's the source of the build failures.
https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core

@mihnita
Copy link
Contributor Author

mihnita commented Nov 21, 2024

@rp9-next
Do you think I should make it 9.0 instead of 10.0?

@mihnita
Copy link
Contributor Author

mihnita commented Nov 21, 2024

Perhaps the arm build is failing due to this change ?

Yes, already fixed (6 hours ago)

@mihnita
Copy link
Contributor Author

mihnita commented Nov 21, 2024

May I ask, how did you get to 10.0 ?

The error pointed me to the right file and line.
I diff-ed the file pre / post update (from a Windows snapshot).

I removed the condition on version and the failure was fixed.
So then I tried to make it pass the test.

I have doubts about 10.0
Maybe should be 9.0. Or maybe v4.8

Was this set through visual studio IDE ?

No, it was "by hand" following this doc:
https://learn.microsoft.com/en-us/cpp/build/how-to-modify-the-target-framework-and-platform-toolset?view=msvc-170#target-framework-ccli-project-only

@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

adding or removing the signature byte sequence (aka BOM)?

@rp9-next
Copy link
Contributor

I have doubts about 10.0
Maybe should be 9.0. Or maybe v4.8

According to https://learn.microsoft.com/en-us/dotnet/standard/frameworks#latest-versions, 9.0 seems like a valid target version. Perhaps 9.0 (or 8.0 if for some reason it doesn't work).
I think this is better fix (vs #3277) if CI gets green.

@mihnita mihnita closed this Nov 21, 2024
@mihnita mihnita deleted the mihai_fix_win_build2 branch November 21, 2024 21:05
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