Bug 2142178

Summary: Review Request: dotnet7.0 - .NET 7 SDK and Runtime
Product: [Fedora] Fedora Reporter: Omair Majid <omajid>
Component: Package ReviewAssignee: Neal Gompa <ngompa13>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: 8ru2u4gz, boost113, jarle, mike, ngompa13, package-review
Target Milestone: ---Flags: ngompa13: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2023-01-16 04:32:55 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Omair Majid 2022-11-11 22:09:48 UTC
Spec URL: https://pagure.io/dotnet-sig/dotnet7.0/raw/main/f/dotnet7.0.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/%40dotnet-sig/dotnet-preview/srpm-builds/05032840/dotnet7.0-7.0.0-0.1.fc36.src.rpm
Description:

.NET is a fast, lightweight and modular platform for creating
cross platform applications that work on Linux, macOS and Windows.

It particularly focuses on creating console applications, web
applications and micro-services.

.NET contains a runtime conforming to .NET Standards a set of
framework libraries, an SDK containing compilers and a 'dotnet'
application to drive everything.

Fedora Account System Username: omajid

Comment 1 Omair Majid 2022-11-11 22:12:07 UTC
This is not an upgrade or replacement of the existing dotnet3.1 or dotnet6.0 packages, but an entirely new package. The packages can all be installed side-by-side.

Here's a diff of the dotnet6.0 and this new dotnet7.0 spec file: hhttps://gist.github.com/omajid/2912c6ceebd7445bc1d4419668cd15e1

This package needs to be bootstrapped. Which can be done by changing the %bcond_without. We have a blanket bootstrap exception from the fedora packaging committee: https://pagure.io/packaging-committee/issue/989.

Comment 2 Neal Gompa 2022-11-13 15:07:38 UTC
Taking this review.

Comment 3 Neal Gompa 2022-11-13 15:13:23 UTC
Initial spec diff review:

* I'd prefer you kept the original changelog fully intact and added a new entry on top rather than blowing away chunks of the changelog
* You should consider making a variable and using it everywhere that "7.0" is used so that it's easy to change and reduces the diff in the future (e.g. %dotnetver or something?)
* Man pages are not guaranteed to have .gz extension (we could change to zstd compression in the future, for example), so use ".*" instead of ".gz"
* Your conditional for the "dotnet" subpackage currently makes it get generated for RHEL < 9 *and* Fedora. Is that intentional?
* Release "0.1%{?dist}" will create upgrade problems. Use "0%{?dist}.1" instead.

Comment 4 Omair Majid 2022-11-14 20:35:28 UTC
Thanks!

Here's the updated package:

Spec URL: https://pagure.io/dotnet-sig/dotnet7.0/raw/main/f/dotnet7.0.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/%40dotnet-sig/dotnet-preview/srpm-builds/05035862/dotnet7.0-7.0.100-0.fc36.1.src.rpm

Copr build: https://copr.fedorainfracloud.org/coprs/g/dotnet-sig/dotnet-preview/build/5035862/

> * I'd prefer you kept the original changelog fully intact and added a new entry on top rather than blowing away chunks of the changelog

I actually started working on this package a long time ago (probably around June). I couldn't propose to add it to Fedora back then because it wouldn't build and/or bootstrap until recently. The changelog diff is just a reflection of that. I can re-add the recent entries from dotnet6.0's changelog if you still prefer that.


> * You should consider making a variable and using it everywhere that "7.0" is used so that it's easy to change and reduces the diff in the future (e.g. %dotnetver or something?)
> * Man pages are not guaranteed to have .gz extension (we could change to zstd compression in the future, for example), so use ".*" instead of ".gz"
> * Release "0.1%{?dist}" will create upgrade problems. Use "0%{?dist}.1" instead.

Thanks, all these should be fixed now.

> * Your conditional for the "dotnet" subpackage currently makes it get generated for RHEL < 9 *and* Fedora. Is that intentional?

Kind of. Back when we added dotnet3.1 to Fedora, we had a `dotnet` subpackage to let users install the latest version of .NET/.NET Core. It was a mistake to have that because it's rarely what users want. Different versions of .NET aren't compatible with each other. Out of the box, a piece of code that wants .NET 6 will simply not work against .NET 7. So users generally want an explicit version of dotnet - and we should help them make that decision explicitly at install time. For RHEL 9, since it was a new OS, we could remove the dotnet subpackage without any issues. But we kept that subpacakge around in Fedora to keep different versions of .NET in different versions of Fedora consistent.

Is it okay if we leave this subpackage out Fedora starting with .NET 7? That means only dotnet6.0 (and earlier) will provide that package and users will get dotnet-sdk-6.0 installed if they ask for `dotnet`.

Comment 5 Neal Gompa 2022-11-15 08:28:26 UTC
(In reply to Omair Majid from comment #4)
> Thanks!
> 
> Here's the updated package:
> 
> Spec URL: https://pagure.io/dotnet-sig/dotnet7.0/raw/main/f/dotnet7.0.spec
> SRPM URL:
> https://download.copr.fedorainfracloud.org/results/%40dotnet-sig/dotnet-
> preview/srpm-builds/05035862/dotnet7.0-7.0.100-0.fc36.1.src.rpm
> 
> Copr build:
> https://copr.fedorainfracloud.org/coprs/g/dotnet-sig/dotnet-preview/build/
> 5035862/
> 
> > * I'd prefer you kept the original changelog fully intact and added a new entry on top rather than blowing away chunks of the changelog
> 
> I actually started working on this package a long time ago (probably around
> June). I couldn't propose to add it to Fedora back then because it wouldn't
> build and/or bootstrap until recently. The changelog diff is just a
> reflection of that. I can re-add the recent entries from dotnet6.0's
> changelog if you still prefer that.
> 
> 

Oh, that makes sense. Don't worry about it then.

> > * You should consider making a variable and using it everywhere that "7.0" is used so that it's easy to change and reduces the diff in the future (e.g. %dotnetver or something?)
> > * Man pages are not guaranteed to have .gz extension (we could change to zstd compression in the future, for example), so use ".*" instead of ".gz"
> > * Release "0.1%{?dist}" will create upgrade problems. Use "0%{?dist}.1" instead.
> 
> Thanks, all these should be fixed now.
> 
> > * Your conditional for the "dotnet" subpackage currently makes it get generated for RHEL < 9 *and* Fedora. Is that intentional?
> 
> Kind of. Back when we added dotnet3.1 to Fedora, we had a `dotnet`
> subpackage to let users install the latest version of .NET/.NET Core. It was
> a mistake to have that because it's rarely what users want. Different
> versions of .NET aren't compatible with each other. Out of the box, a piece
> of code that wants .NET 6 will simply not work against .NET 7. So users
> generally want an explicit version of dotnet - and we should help them make
> that decision explicitly at install time. For RHEL 9, since it was a new OS,
> we could remove the dotnet subpackage without any issues. But we kept that
> subpacakge around in Fedora to keep different versions of .NET in different
> versions of Fedora consistent.
> 
> Is it okay if we leave this subpackage out Fedora starting with .NET 7? That
> means only dotnet6.0 (and earlier) will provide that package and users will
> get dotnet-sdk-6.0 installed if they ask for `dotnet`.

It seems weird that we'd drop it though, given that we have the dotnet-host package generated too. I think it makes sense to have an unversioned package that gives you the latest one. That said, it does make sense to me that RHEL wouldn't have this anymore, given that you'll keep adding new versions across the same major RHEL version.

But if you really want to drop it, the conditional is not very clear on that. And all the other dotnet packages will need cleanup for that too...

Comment 6 Omair Majid 2022-11-15 16:36:44 UTC
> It seems weird that we'd drop it though, given that we have the dotnet-host package generated too.

So `dotnet-host` provides an entry point (`/usr/bin/dotnet`) to running all .NET versions. Think of it like python's `venv` and `python3` all rolled into one command. dotnet-host also provides man pages and a bunch of other things. If we don't ship it we wont have .NET working at all. And we only really need to ship one version of that: the version from .NET 7 will handle .NET 6 just fine.

> That said, it does make sense to me that RHEL wouldn't have this anymore, given that you'll keep adding new versions across the same major RHEL version.

I plan to do the same for Fedora. Fedora 36 has .NET Core 3.1 (dotnet3.1), .NET 6.0 (dotnet6.0) and I will be adding this .NET 7 (dotnet7.0) to Fedora 36 and later.

There just isn't a good reason for `dotnet` to exist as a separate package when everyone (including upstream) expects users to install versions explicitly.

> But if you really want to drop it, the conditional is not very clear on that. 

I can fix that shortly with better comments, if you want.

> And all the other dotnet packages will need cleanup for that too...

Super long term, I do want to stop providing a plain unversioned `dotnet` package everywhere, including Fedora.

Would you be okay if I left dotnet3.1 and dotnet6.0 continue providing a `dotnet` package and just remove it from dotnet7.0? Or would you prefer we handle Fedora 38 and later differently than Fedora 37 and earlier?

Comment 8 Neal Gompa 2022-11-21 16:39:56 UTC
(In reply to Omair Majid from comment #6)
> > It seems weird that we'd drop it though, given that we have the dotnet-host package generated too.
> 
> So `dotnet-host` provides an entry point (`/usr/bin/dotnet`) to running all
> .NET versions. Think of it like python's `venv` and `python3` all rolled
> into one command. dotnet-host also provides man pages and a bunch of other
> things. If we don't ship it we wont have .NET working at all. And we only
> really need to ship one version of that: the version from .NET 7 will handle
> .NET 6 just fine.
> 
> > That said, it does make sense to me that RHEL wouldn't have this anymore, given that you'll keep adding new versions across the same major RHEL version.
> 
> I plan to do the same for Fedora. Fedora 36 has .NET Core 3.1 (dotnet3.1),
> .NET 6.0 (dotnet6.0) and I will be adding this .NET 7 (dotnet7.0) to Fedora
> 36 and later.
> 
> There just isn't a good reason for `dotnet` to exist as a separate package
> when everyone (including upstream) expects users to install versions
> explicitly.
> 
> > But if you really want to drop it, the conditional is not very clear on that. 
> 
> I can fix that shortly with better comments, if you want.
> 
> > And all the other dotnet packages will need cleanup for that too...
> 
> Super long term, I do want to stop providing a plain unversioned `dotnet`
> package everywhere, including Fedora.
> 
> Would you be okay if I left dotnet3.1 and dotnet6.0 continue providing a
> `dotnet` package and just remove it from dotnet7.0? Or would you prefer we
> handle Fedora 38 and later differently than Fedora 37 and earlier?

I'd prefer F38+ differently from older versions. You'll probably want to fix up the developer.fedoraproject.org content for this new reality, though.

Comment 10 Omair Majid 2022-11-21 21:32:30 UTC
I will do a pass on developer.fedoraproject.org after this package is approved. I looked over it and it needs more changes than just `dotnet` to `dotnet-sdk-7.0` rename; it still talks about .NET Core 2.1 (which has been EOL for years) and dotnet-sig copr repos, which are unnecessary for all current Fedora releases.

Comment 11 Neal Gompa 2022-11-22 00:52:24 UTC
> %patch1 -p1
> %patch2 -p1
> %patch3 -p1
> %patch4 -p1
> %patch5 -p1
> %patch6 -p1
> %patch7 -p1
> %patch8 -p1
> %patch9 -p1
> %patch10 -p1
> %patch11 -p1

Any reason you're not just using "%autopatch -p1" here?

Comment 12 Neal Gompa 2022-11-22 01:05:14 UTC
> License:        MIT and ASL 2.0 and BSD and LGPLv2+ and CC-BY and CC0 and MS-PL and EPL-1.0 and GPL+ and GPLv2 and ISC and OFL and zlib

You might want to go ahead and convert this to SPDX notation now. Note that "CC0" is banned for code in Fedora nowadays, so if there's still CC0 code, you'll need an exception from FE-Legal. 

Cf. https://docs.fedoraproject.org/en-US/legal/update-existing-packages/

Cf. https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/message/RRYM3CLYJYW64VSQIXY6IF3TCDZGS6LM/

Comment 13 Omair Majid 2022-11-22 16:17:31 UTC
> Any reason you're not just using "%autopatch -p1" here?

Thanks for catching this. We didn't use this in .NET 6 because we had to `pushd`/`popd` into the correct directory first before applying patches. But that's no longer needed.

> Note that "CC0" is banned for code in Fedora nowadays, so if there's still CC0 code, you'll need an exception from FE-Legal. 

It looks like only these code files have a mention of creative commons:

https://github.com/dotnet/runtime/tree/main/src/libraries/System.Private.CoreLib/src/System/Random.Xoshiro128StarStarImpl.cs
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Private.CoreLib/src/System/Random.Xoshiro256StarStarImpl.cs

https://github.com/dotnet/fsharp/tree/main/tests/benchmarks/CompiledCodeBenchmarks/TaskPerf/TaskPerf/TaskBuilder.fs
https://github.com/dotnet/fsharp/tree/main/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/Tasks.fs
https://github.com/dotnet/fsharp/tree/main/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/TasksDynamic.fs

https://github.com/dotnet/runtime/tree/main/src/mono/mono/utils/dlmalloc.c
https://github.com/dotnet/runtime/tree/main/src/mono/mono/utils/dlmalloc.h

The first two have a MIT license header in addition to CC0. Is any action needed on those? Can I even "ignore" that they are under CC0 because of the MIT license header?

The next 3 don't have an MIT or Apache header. But they are tests (that aren't run during the build). I suppose I can just delete these files?

The final 2 are dlmalloc. This seems to be an older version of dlmalloc that uses the language that makes it CC-PDDC, which sounds acceptable to Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=2120577#c1 ?

Comment 15 Omair Majid 2022-11-23 17:31:35 UTC
While we wait for the discussion on the legal mailing list, are there any other issues we should resolve? Is this package okay to go as soon as the legal discussion is sorted out and we have some sort of approval?

Comment 16 Neal Gompa 2022-11-23 18:22:27 UTC
No, I think we're in good shape after this. I'm holding off on granting the review until we have that figured out.

Comment 17 Omair Majid 2022-11-25 16:02:27 UTC
As part of doing the conversion to SDPX, I ran into more issues:

Bugs in license-validate
- WITH operator: https://pagure.io/copr/license-validate/issue/7
- BSD-4-Clause-UC: https://pagure.io/copr/license-validate/issue/8
- bzip2-1.0.6: https://pagure.io/copr/license-validate/issue/9
- LicenseRef-ISO-8879: https://pagure.io/copr/license-validate/issue/10

Missing SPDX identifiers in Fedora license list:
- SAX-PD: https://gitlab.com/fedora/legal/fedora-license-data/-/issues/99
- W3C-19980720: https://gitlab.com/fedora/legal/fedora-license-data/-/issues/100

More questions, added to the existing thread on legal@ :  https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/message/LAX6CZBKR3JFCLTUHHOJWSX3MV4XLSGA/

Comment 18 jarle 2023-01-02 13:14:43 UTC
Is there any progress on this ?

Comment 19 Omair Majid 2023-01-07 17:24:14 UTC
Spec URL: https://pagure.io/dotnet-sig/dotnet7.0/raw/main/f/dotnet7.0.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/%40dotnet-sig/dotnet-preview/srpm-builds/05205578/dotnet7.0-7.0.100-0.fc36.1.src.rpm

I am aware of 3 follow-up items:

- Add this MIT variant to SPDX and fedora-license-data: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/AttRegexTests.cs. For now, I have worked with the assumption that this falls in the SPDX approach that MIT-is-a-category-of-licenses and just used MIT identifier for Fedora.
- Add this MIT variant to SPDX/fedora-license-data: https://fedoraproject.org/wiki/Licensing:MIT?rd=Licensing/MIT#HP_Variant. Same as above.
- https://gitlab.com/fedora/legal/fedora-license-data/-/issues/104#note_1230640823. For now, I have ripped out that test code that copy/pasted from StackOverflow.

For CC0-1.0 in particular, the exception at https://gitlab.com/fedora/legal/fedora-license-data/-/blob/main/data/CC0-1.0.toml#L11 applies to .NET 7 since it's a newer version of .NET which was already included in Fedora (as dotnet3.1, and dotnet6.0, etc).

With that, I believe all legal issues have been sorted out. Is it okay to include this package in Fedora now?

Comment 20 Jakub Kadlčík 2023-01-07 21:49:46 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/5206160
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2142178-dotnet7.0/fedora-rawhide-x86_64/05206160-dotnet7.0/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

Comment 21 Neal Gompa 2023-01-07 23:36:00 UTC
At this point, this package looks good to me now.

PACKAGE APPROVED.

Comment 22 Gwyn Ciesla 2023-01-09 15:31:51 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/dotnet7.0

Comment 23 Omair Majid 2023-01-16 04:32:55 UTC
Thanks for the review and the approval!

I have built dotnet7.0 for all currently supported Fedora versions:

Rawhide: https://bodhi.fedoraproject.org/updates/FEDORA-2023-0fee44158c
Fedora 37: https://bodhi.fedoraproject.org/updates/FEDORA-2023-27a1022e1f
Fedora 36: https://bodhi.fedoraproject.org/updates/FEDORA-2023-3d309b302f