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

Update Microsoft.Extensions.Logging to v7.0.0, drop netcoreap3.1 support #1990

Merged
merged 13 commits into from
Jan 6, 2023

Conversation

pgrawehr
Copy link
Contributor

@pgrawehr pgrawehr commented Dec 3, 2022

Fixes #1984

Updates Microsoft.Extensions.Logging and Microsoft.Extensions.Logging.Abstractions to v7.0.0. Since they officially no longer support netcoreapp3.1, that is dropped now, as discussed in #1984.

All tests are now by default built against .net6.0, some additionally against .net48 (to test netstandard2.0 support).

Microsoft Reviewers: Open in CodeFlow

Copy link
Contributor

@raffaeler raffaeler left a comment

Choose a reason for hiding this comment

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

Not sure if the sendTohelix.proj, Iot.Device.Bindings.csproj and System.Device.Gpio.Tests.csproj can have the parametrized tfms.
Out of that, it looks great.

I don't get why the onewire ReadTemperatureAsync fails as it is fenced from netstandard2.0.

Causes a compatibility regression when a netcoreapp3.1 application
now reverts to use the netstandard2.0 library.
(Another instance of the macos compiler failure that occurs if
two projects share the same physical folder)
@pgrawehr
Copy link
Contributor Author

pgrawehr commented Dec 4, 2022

Not sure if the sendTohelix.proj, Iot.Device.Bindings.csproj and System.Device.Gpio.Tests.csproj can have the parametrized tfms. Out of that, it looks great.

Appears to work. The helix tests don't work yet, but that's because it cannot find xunit-console.exe, or maybe even it sets up an incorrect command line. But I don't know the infrastructure there well enough to be able to fix this one.

I don't get why the onewire ReadTemperatureAsync fails as it is fenced from netstandard2.0.

There's an #ifdef !netstandard2.0 in the binding. The compatibility testing tests whether the new build is backwards compatible, which it is not for a netcoreapp3.1 assembly, because it now has to link against the netstandard-build, which does not have those methods.

@raffaeler
Copy link
Contributor

There's an #ifdef !netstandard2.0 in the binding

I've seen that, that's why I commented that looked good to me.
I guess the rest of the work should be done with higher privileges in the repo.

@pgrawehr
Copy link
Contributor Author

pgrawehr commented Dec 4, 2022

@joperezr Can you take a look at the helix build failures? It seems we must somehow choose a new path for the xunit-console runner, but I don't know how I can find that.

@@ -12,7 +12,7 @@

<IncludeDotNetCli>true</IncludeDotNetCli>
<DotNetCliPackageType>sdk</DotNetCliPackageType>
<DotNetCliVersion>3.1.405</DotNetCliVersion>
<DotNetCliVersion>6.0.400</DotNetCliVersion>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that you can make the changes to this file like you are doing here. This depends on on some Helix infrastructure, so we should make sure which version can be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part appears to be working (see log from build), the problem is that it's not finding a matching xunit-console binary. And I don't know where that one is referenced (this project file only mentions the arguments to it, but not the binary itself, it seems)

@@ -12,11 +12,11 @@
<SystemRuntimeWindowsRuntimePackageVersion>4.6.0</SystemRuntimeWindowsRuntimePackageVersion>
<SystemManagementPackageVersion>5.0.0</SystemManagementPackageVersion>
<SystemThreadingTasksExtensionsPackageVersion>4.5.4</SystemThreadingTasksExtensionsPackageVersion>
<SystemMemoryPackageVersion>4.5.4</SystemMemoryPackageVersion>
<SystemMemoryPackageVersion>4.5.5</SystemMemoryPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we have to make these changes together with the nca3.1 changes? I think it would be better to isolate the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually the other way round: The package updates require the nca3.1 changes, because the newer packages are not supported on nca3.1 any more.

It's probably possible to separate the two (and merge the 3.1 change first), if that's really required.

@joperezr joperezr added the Priority:0 Work that we can't release without label Dec 8, 2022
@pgrawehr
Copy link
Contributor Author

/azp run dotnet.iot

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pgrawehr
Copy link
Contributor Author

@MattGal Can you help here?

I have updated the tests to use .net6.0, which includes the helix tests as well. But unfortunately, something is missing (or incorrect) now. Running the tests results in this error (full build log here):

+ sudo -E /home/helixbot/work/B9EC0A33/p/dotnet-cli/dotnet /home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0/xunit.console.exe System.Device.Gpio.Tests.dll -nocolor -verbose -serialize -maxthreads 1 -notrait SkipOnTestRun=Unix -xml testResults.xml
....

  * You intended to execute a .NET program, but dotnet-/home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0

Apparently, the path /home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0/xunit.console.exe is wrong. Where should that be and how can it be changed? I can't seem to see that path in any of the build scripts.

@MattGal
Copy link
Member

MattGal commented Dec 29, 2022

@MattGal Can you help here?

I have updated the tests to use .net6.0, which includes the helix tests as well. But unfortunately, something is missing (or incorrect) now. Running the tests results in this error (full build log here):

+ sudo -E /home/helixbot/work/B9EC0A33/p/dotnet-cli/dotnet /home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0/xunit.console.exe System.Device.Gpio.Tests.dll -nocolor -verbose -serialize -maxthreads 1 -notrait SkipOnTestRun=Unix -xml testResults.xml
....

  * You intended to execute a .NET program, but dotnet-/home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0

Apparently, the path /home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0/xunit.console.exe is wrong. Where should that be and how can it be changed? I can't seem to see that path in any of the build scripts.

Just got back from holiday time, I will take a look at this and let you know if I have any useful advice.

@MattGal
Copy link
Member

MattGal commented Dec 29, 2022

@MattGal Can you help here?
I have updated the tests to use .net6.0, which includes the helix tests as well. But unfortunately, something is missing (or incorrect) now. Running the tests results in this error (full build log here):

+ sudo -E /home/helixbot/work/B9EC0A33/p/dotnet-cli/dotnet /home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0/xunit.console.exe System.Device.Gpio.Tests.dll -nocolor -verbose -serialize -maxthreads 1 -notrait SkipOnTestRun=Unix -xml testResults.xml
....

  * You intended to execute a .NET program, but dotnet-/home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0

Apparently, the path /home/helixbot/work/B9EC0A33/p/xunit-runner/tools/net6.0/xunit.console.exe is wrong. Where should that be and how can it be changed? I can't seem to see that path in any of the build scripts.

Just got back from holiday time, I will take a look at this and let you know if I have any useful advice.

OK so what's going on here is your choice of "runtimeTargetFramework" of net6.0, which I don't think is the right value.

The Arcade Helix SDK's support for XUnit tests is pretty simple; it includes the nupkg directly (it's just a zip after all) as a correlation payload, and then in the Arcade sources here you can see how it calculates where in this nupkg the xunit console runner lives.

Unpacking the raw XUnit runner package you'll see there just isn't a runtimeTargetFramework with that name, so the path it tries to construct based off your changes doesn't exist and you get the error you're noting.

I think just changing runtimeTargetFramework to netcoreapp2.0 should be all that's needed here. I'll leave a suggested change for that.

eng/sendToHelix.proj Outdated Show resolved Hide resolved
Co-authored-by: Matt Galbraith <MattGal@users.noreply.github.com>
@joperezr
Copy link
Member

joperezr commented Jan 5, 2023

/azp run dotnet.iot

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@joperezr
Copy link
Member

joperezr commented Jan 5, 2023

Looks like #1916 added some merge conflicts so we can't merge this as is any longer. @pgrawehr could you please resolve and merge?

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Other than the merge conflicts that need to be fixed, this looks good to me.

# Conflicts:
#	src/devices/Display/Display.sln
@pgrawehr pgrawehr merged commit cc07422 into dotnet:main Jan 6, 2023
@pgrawehr pgrawehr deleted the Logging-7.0 branch January 6, 2023 14:42
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Priority:0 Work that we can't release without
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shall we deprecate netcoreapp3.1 build?
4 participants