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

Target net8.0 for source-build #5193

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Conversation

NikolaMilosavljevic
Copy link
Contributor

Bug

Fixes: NuGet/Home#12620

Regression? No

Description

NuGet.Client needs to target net8.0 for source-build in order to eliminate all prebuilts. Source-build must build the complete dependency graph with no external dependencies. We don't allow targeting net7 or (n-1) as is would require building all of .NET 7.0 from source which would snowball.

@NikolaMilosavljevic NikolaMilosavljevic requested a review from a team as a code owner May 31, 2023 16:06
@ghost ghost added the Community PRs created by someone not in the NuGet team label May 31, 2023
@NikolaMilosavljevic
Copy link
Contributor Author

@NikolaMilosavljevic
Copy link
Contributor Author

@jeffkl @zivkan can this be approved for CI?

@MichaelSimons
Copy link
Contributor

@NikolaMilosavljevic - do you know why the net7.0 related prebuilts were not showing up in #5190?

@NikolaMilosavljevic
Copy link
Contributor Author

@NikolaMilosavljevic - do you know why the net7.0 related prebuilts were not showing up in #5190?

I don't know - #5190 has all the prebuilts that were restored as part of source-build. Investigating...

@NikolaMilosavljevic
Copy link
Contributor Author

@NikolaMilosavljevic - do you know why the net7.0 related prebuilts were not showing up in #5190?

I don't know - #5190 has all the prebuilts that were restored as part of source-build. Investigating...

@MichaelSimons which 7.0 specific prebuilts were we expecting to see in #5190?

@MichaelSimons
Copy link
Contributor

@NikolaMilosavljevic - do you know why the net7.0 related prebuilts were not showing up in #5190?

I don't know - #5190 has all the prebuilts that were restored as part of source-build. Investigating...

@MichaelSimons which 7.0 specific prebuilts were we expecting to see in #5190?

Not certain but I am thinking the following:

    <Usage Id="Microsoft.AspNetCore.App.Ref" Version="7.0.5" />
    <Usage Id="Microsoft.NETCore.App.Host.linux-x64" Version="7.0.5" />
    <Usage Id="Microsoft.NETCore.App.Ref" Version="7.0.5" />

@NikolaMilosavljevic
Copy link
Contributor Author

@NikolaMilosavljevic - do you know why the net7.0 related prebuilts were not showing up in #5190?

I don't know - #5190 has all the prebuilts that were restored as part of source-build. Investigating...

@MichaelSimons which 7.0 specific prebuilts were we expecting to see in #5190?

Not certain but I am thinking the following:

    <Usage Id="Microsoft.AspNetCore.App.Ref" Version="7.0.5" />
    <Usage Id="Microsoft.NETCore.App.Host.linux-x64" Version="7.0.5" />
    <Usage Id="Microsoft.NETCore.App.Ref" Version="7.0.5" />

I'm pretty certain that these are resolved from the restored SDK, using KnownFrameworkReference. I do not see any of these packages in binlog, artifacts folder or nuget cache.

@MichaelSimons
Copy link
Contributor

I'm pretty certain that these are resolved from the restored SDK, using KnownFrameworkReference.

Agreed. Ideally we would be able to detect these. The arcade prebuilt infra does. I see this as being outside the scope of this PR regardless.

@MichaelSimons
Copy link
Contributor

I think it would be good to create a source-build patch and PR so that the changes can be validated in the context of the full product build to ensure the prebuilts have been resolved.

@NikolaMilosavljevic
Copy link
Contributor Author

I think it would be good to create a source-build patch and PR so that the changes can be validated in the context of the full product build to ensure the prebuilts have been resolved.

Yes, working on it.

@zivkan
Copy link
Member

zivkan commented Jun 1, 2023

I'm a bit confused about all the conversation that's been going on here. I think it's orthogonal to this PR and the conversation could have been elsewhere (but something in this PR triggered the question).

Or is the conversation about something that should block the PR, so we shouldn't merge it yet?

@MichaelSimons
Copy link
Contributor

I'm a bit confused about all the conversation that's been going on here. I think it's orthogonal to this PR and the conversation could have been elsewhere (but something in this PR triggered the question).

The question is why the source-build repo level prebuilt detection didn't report the .NET 7.0 targeting packages. The answer is because NuGet.Client does not utilize Arcade so the prebuilt detection infra is not as good. This is an orthogonal issue that should not block this PR. Please merge when appropriate.

@NikolaMilosavljevic
Copy link
Contributor Author

Let's hold off with merge until we understand if additional change is needed. We are running this same change in full source-build, as part of dotnet/installer#16541. There is an error about obsolete API when compiling with Preview 4 SDK.

@NikolaMilosavljevic
Copy link
Contributor Author

Let's hold off with merge until we understand if additional change is needed. We are running this same change in full source-build, as part of dotnet/installer#16541. There is an error about obsolete API when compiling with Preview 4 SDK.

I've created an issue to track this in nuget.client repo: NuGet/Home#12626

I am adding a workaround for full source-build: dotnet/installer@3191211

@NikolaMilosavljevic
Copy link
Contributor Author

@jeffkl @zivkan is it possible to rerun the checks? All test failures are in machine setup steps and unrelated to any repo changes.

@jeffkl
Copy link
Contributor

jeffkl commented Jun 7, 2023

@jeffkl @zivkan is it possible to rerun the checks? All test failures are in machine setup steps and unrelated to any repo changes.

The End to End test failures have been fixed but you'll need to rebase. Ping me when you've pushed and I can start the CI.

@NikolaMilosavljevic
Copy link
Contributor Author

@jeffkl @zivkan is it possible to rerun the checks? All test failures are in machine setup steps and unrelated to any repo changes.

The End to End test failures have been fixed but you'll need to rebase. Ping me when you've pushed and I can start the CI.

Tried to rebase with 'close/reopen' but that doesn't seem to work in this repo.

@jeffkl I've pushed the rebased changes.

@jeffkl jeffkl merged commit 8fc60c5 into NuGet:dev Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NuGet.Client needs to target net8.0 for source-build
4 participants