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

Bump lang version to 10.0 #375

Merged
merged 6 commits into from
Feb 23, 2022
Merged

Bump lang version to 10.0 #375

merged 6 commits into from
Feb 23, 2022

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Feb 18, 2022

#351 leftovers

Changes proposed in this pull request:
Bump supported lang version to the latest (10.0).

10.0 version is also used in .NET OTel SDK repo.

When the previous PR was created there were not support for .NET 6/Lang 10 in our pipeline.

which i 10.0 right now
@Kielek Kielek requested a review from a team February 18, 2022 10:24
@Kielek
Copy link
Contributor Author

Kielek commented Feb 18, 2022

Consider to disable whitespace diff when you are checking diff.

Directory.Build.props Outdated Show resolved Hide resolved
@@ -6,108 +6,107 @@
using OpenTelemetry.Exporter;
using Xunit;

namespace OpenTelemetry.AutoInstrumentation.Tests.Configuration
namespace OpenTelemetry.AutoInstrumentation.Tests.Configuration;
Copy link
Member

Choose a reason for hiding this comment

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

How about reformatting the whole codebase for sake of consistency?

Maybe it is only me, but I favor consistency when I am reading code. Even when new features reduces some noise. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do it, in this PR?

Copy link
Member

@pellared pellared Feb 18, 2022

Choose a reason for hiding this comment

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

As for me. YES 👍 Especially that there are no open PRs touching the code.

And it would be good to merge on timely manner to reduce possible conflicts.

@pellared pellared changed the title bump lang version to the latest Bump lang version to 10.0 Feb 18, 2022
@Kielek
Copy link
Contributor Author

Kielek commented Feb 18, 2022

Guys.

  1. .NET 4.6.1 end of support April 26.
  2. Windows 2022 virtual env for GithubActions does not support .NET 4.6.1 framework
  3. Windows 2019 virtual env for GithubActions does not support lang version 10.0 in some cases.

Possible solutions

  1. Use LangVer - latest intead of 10.0 - it should be working
  2. Learn how to install TargetingFramework for .NET 4.6.1 on Win2022.
  3. Try to fix compilation issues in Win 2019.
  4. Drop support for .NET 4.6.1, use .NET 4.6.2, build on Win 2022.

Do you have any preferences?

@pellared
Copy link
Member

10.0 version is also used in .NET OTel SDK repo.

Can you check how they solved it as they should also support .NET 4.6.1?

@Kielek
Copy link
Contributor Author

Kielek commented Feb 21, 2022

The issue is related to using MSBuild task (it is taking version from the installed VS) from Nuke instead of DotnetMsBuild Task (taking msbuild included into dotnet).

For most cases, I was able to fix it, the only leftover is https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/tree/main/test/test-applications/integrations/dependency-libs .
It is referenced by the .vcxproj folder and cannot be easily converted to the the DotnetMsBuild due to following commend

// Can't use dotnet msbuild, as needs to use the VS version of MSBuild

Previous comment is still valid. We can bump environment to windows-2022 but it requires eitjer

@pellared
Copy link
Member

But everything works for fine windows-2019 so we could merge as it is? If so I am fine merging as it is.

I created a separate issue where we can discuss .NET Framework 4.6.1 support #376

@Kielek
Copy link
Contributor Author

Kielek commented Feb 21, 2022

@pellared, yes, IMO it is ready to be merged. Some improvements can be done later.

@pellared pellared requested review from nrcventura and RassK February 23, 2022 08:55
@nrcventura nrcventura merged commit ca40891 into open-telemetry:main Feb 23, 2022
@Kielek Kielek deleted the bump-lang-version branch February 23, 2022 18:32
@Kielek Kielek mentioned this pull request Aug 23, 2022
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.

4 participants