Bug 2142178
Summary: | Review Request: dotnet7.0 - .NET 7 SDK and Runtime | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Omair Majid <omajid> |
Component: | Package Review | Assignee: | Neal Gompa <ngompa13> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
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. Taking this review. 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. 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`. (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... > 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 updated the comment and conditionals for building the `dotnet` subpackage. Spec diff: https://pagure.io/dotnet-sig/dotnet7.0/c/f78d53f5cc4f7ddbf05bbc491c6bb33d2d3635a5?branch=main 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/05043853/dotnet7.0-7.0.100-0.fc36.1.src.rpm Build: https://copr.fedorainfracloud.org/coprs/g/dotnet-sig/dotnet-preview/build/5043853/ (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. 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/05057773/dotnet7.0-7.0.100-0.fc36.1.src.rpm Spec diff: https://pagure.io/dotnet-sig/dotnet7.0/c/de2f31ceab56d5a754d031257301d856b89ce690?branch=main Build: https://copr.fedorainfracloud.org/coprs/g/dotnet-sig/dotnet-preview/build/5057773/ 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. > %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?
> 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/ > 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 ? Started a thread on legal.org : https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/UEKBN4EUI4I4JEUMKPZJGPEL72ZUV2OH/ 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? No, I think we're in good shape after this. I'm holding off on granting the review until we have that figured out. 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/ Is there any progress on this ? 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? 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 At this point, this package looks good to me now. PACKAGE APPROVED. (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/dotnet7.0 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 |