Spec URL: https://sgallagh.fedorapeople.org/packagereview/modular-release/modular-release.spec SRPM URL: https://sgallagh.fedorapeople.org/packagereview/modular-release/modular-release-26-0.2.fc26.src.rpm Description: Fedora release files such as various %{_sysconfdir}/ files that define the modular release. Fedora Account System Username: sgallagh
Some additional data about this package: * It copies the license files from fedora-release verbatim. * The presets were generated by getting a list of the units provided by the base runtime package set ( https://github.com/fedora-modularity/whatpkgs/blob/master/sampledata/fedora/26/runtime-binary-dependency-packages-short.txt ) and filtering the 90-default.preset file from fedora-release down to just those referenced there.
This should be prefixed with fedora- in its name. This is *not* generic.
* modular-release.spec:1-3 %global is generally preferred over %define; let's switch to that * 90-default.preset: I'd remove the majority of the `enable' presets as nothing in the Base Runtime actually provides such functionality (or in some cases, unit files). Perhaps your filter mentioned in comment #1 didn't quite work? Or maybe I'm doing it wrong :) The one I'd remove include: - bluetooth (we only have the grouping target) - avahi-daemon (we don't have avahi) - cups (we don't have cups) - rsyslog (we don't have rsyslog) - syslog-ng (we don't have syslog-ng) - sysklogd (nothing in Fedora provides this) - gpm (we don't have gpm) - mcelog (we don't have mcelog) * modular-release.spec:44-48 I assume the reason for creating fedora-release and system-release-cpe under %{_prefix} (which is not what fedora-release does today) is to unify where all the release files are kept, as issue, issue.net, os-release and os.release.d are already there. Is that right? Note fedora-release doesn't do this. * The standard fedora-release package also installs %{_prefix}/lib/os.release.d, plus some files/links under it. Don't we want it too? * Additionaly, it also installs %{_prefix}/lib/variant. Should we have it too?
(In reply to Neal Gompa from comment #2) > This should be prefixed with fedora- in its name. This is *not* generic. Note this package is only meant for the module-based compose and it is the only variant we're going to ship. Putting "fedora" in the name feels redundant to me, even if its content in Fedora dist-git includes files specific to Fedora. Also note this package won't be part of the traditional release -- it will be excluded from the compose.
(In reply to Petr Šabata from comment #3) > * modular-release.spec:1-3 > %global is generally preferred over %define; let's switch to that > Done. > * 90-default.preset: > I'd remove the majority of the `enable' presets as nothing in the > Base Runtime actually provides such functionality (or in some cases, > unit files). Perhaps your filter mentioned in comment #1 didn't quite > work? Or maybe I'm doing it wrong :) The one I'd remove include: > - bluetooth (we only have the grouping target) > - avahi-daemon (we don't have avahi) > - cups (we don't have cups) > - rsyslog (we don't have rsyslog) > - syslog-ng (we don't have syslog-ng) > - sysklogd (nothing in Fedora provides this) > - gpm (we don't have gpm) > - mcelog (we don't have mcelog) > I think you're right that my filter may have been wrong. I've removed those. > * modular-release.spec:44-48 > I assume the reason for creating fedora-release and system-release-cpe > under > %{_prefix} (which is not what fedora-release does today) is to unify where > all the release files are kept, as issue, issue.net, os-release and > os.release.d are already there. Is that right? Note fedora-release > doesn't do this. > Yes, this is in preparation of the mythical future where empty /etc is possible. It's been requested of the fedora-release package, but no one has done the work yet. I figured it was okay for us to just do it. > * The standard fedora-release package also installs > %{_prefix}/lib/os.release.d, plus some files/links under it. > Don't we want it too? > > * Additionaly, it also installs %{_prefix}/lib/variant. Should we have it > too? These are related. They exist specifically for dealing with the variants like Fedora Server Edition vs. Fedora Workstation Edition. We don't need these because we won't have variant versions of Base Runtime that need to coexist in the same repository. (If you want an exhaustive explanation, read https://sgallagh.wordpress.com/2016/03/18/sausage-factory-multiple-edition-handling-in-fedora/ )
Spec: https://sgallagh.fedorapeople.org/packagereview/modular-release/modular-release.spec SRPM: https://sgallagh.fedorapeople.org/packagereview/modular-release/modular-release-26-0.3.fc26.src.rpm
(In reply to Petr Šabata from comment #4) > (In reply to Neal Gompa from comment #2) > > This should be prefixed with fedora- in its name. This is *not* generic. > > Note this package is only meant for the module-based compose and it is the > only variant we're going to ship. Putting "fedora" in the name feels > redundant to me, even if its content in Fedora dist-git includes files > specific to Fedora. > > Also note this package won't be part of the traditional release -- it will > be excluded from the compose. The name in the Summary indicates it's Fedora specific. All fedora branding packages are named fedora-*. This should be no different.
(In reply to Stephen Gallagher from comment #5) > (In reply to Petr Šabata from comment #3) > > * modular-release.spec:1-3 > > %global is generally preferred over %define; let's switch to that > > Done. Ack. > > * 90-default.preset: > > I'd remove the majority of the `enable' presets as nothing in the > > Base Runtime actually provides such functionality (or in some cases, > > unit files). Perhaps your filter mentioned in comment #1 didn't quite > > work? Or maybe I'm doing it wrong :) The one I'd remove include: > > - bluetooth (we only have the grouping target) > > - avahi-daemon (we don't have avahi) > > - cups (we don't have cups) > > - rsyslog (we don't have rsyslog) > > - syslog-ng (we don't have syslog-ng) > > - sysklogd (nothing in Fedora provides this) > > - gpm (we don't have gpm) > > - mcelog (we don't have mcelog) > > I think you're right that my filter may have been wrong. I've removed those. Ack. > > * modular-release.spec:44-48 > > I assume the reason for creating fedora-release and system-release-cpe > > under > > %{_prefix} (which is not what fedora-release does today) is to unify where > > all the release files are kept, as issue, issue.net, os-release and > > os.release.d are already there. Is that right? Note fedora-release > > doesn't do this. > > Yes, this is in preparation of the mythical future where empty /etc is > possible. It's been requested of the fedora-release package, but no one has > done the work yet. I figured it was okay for us to just do it. Okay, sounds good. > > * The standard fedora-release package also installs > > %{_prefix}/lib/os.release.d, plus some files/links under it. > > Don't we want it too? > > > > * Additionaly, it also installs %{_prefix}/lib/variant. Should we have it > > too? > > These are related. They exist specifically for dealing with the variants > like Fedora Server Edition vs. Fedora Workstation Edition. We don't need > these because we won't have variant versions of Base Runtime that need to > coexist in the same repository. > > (If you want an exhaustive explanation, read > https://sgallagh.wordpress.com/2016/03/18/sausage-factory-multiple-edition- > handling-in-fedora/ ) Alright. If it breaks something, we'll fix it. I've just noticed your %changelog entries are malformed; probably because your editor doesn't evaluate the macro when inserting the version. (In reply to Neal Gompa from comment #7) > (In reply to Petr Šabata from comment #4) > > (In reply to Neal Gompa from comment #2) > > > This should be prefixed with fedora- in its name. This is *not* generic. > > > > Note this package is only meant for the module-based compose and it is the > > only variant we're going to ship. Putting "fedora" in the name feels > > redundant to me, even if its content in Fedora dist-git includes files > > specific to Fedora. > > > > Also note this package won't be part of the traditional release -- it will > > be excluded from the compose. > > The name in the Summary indicates it's Fedora specific. All fedora branding > packages are named fedora-*. This should be no different. The idea here was that we wouldn't have to change the package name, bootstrap tags and installation & buildroot profiles when [re]building modules outside of Fedora. Of course this isn't the only way to achieve that; we could rename it or package this differently, if you really insist on having Fedora in the package name.
Spec: https://sgallagh.fedorapeople.org/packagereview/modular-release/modular-release.spec SRPM: https://sgallagh.fedorapeople.org/packagereview/modular-release/modular-release-26-0.4.fc26.src.rpm
Changelog fixed.
(In reply to Petr Šabata from comment #8) > (In reply to Neal Gompa from comment #7) > > (In reply to Petr Šabata from comment #4) > > > (In reply to Neal Gompa from comment #2) > > > > This should be prefixed with fedora- in its name. This is *not* generic. > > > > > > Note this package is only meant for the module-based compose and it is the > > > only variant we're going to ship. Putting "fedora" in the name feels > > > redundant to me, even if its content in Fedora dist-git includes files > > > specific to Fedora. > > > > > > Also note this package won't be part of the traditional release -- it will > > > be excluded from the compose. > > > > The name in the Summary indicates it's Fedora specific. All fedora branding > > packages are named fedora-*. This should be no different. > > The idea here was that we wouldn't have to change the package name, > bootstrap tags and installation & buildroot profiles when [re]building > modules outside of Fedora. Of course this isn't the only way to achieve > that; we could rename it or package this differently, if you really insist > on having Fedora in the package name. I do, especially since this conflicts with the regular "fedora-release" and calls itself "Fedora modular release". I firmly believe it should be clear for Fedora branding so that downstream consumers of Fedora have a clear direction on what packages they need to replace, even when making their own "modular" distributions. Please rename this package. If you want to also produce a "generic" version for people to use as a template for downstream use (which would be greatly appreciated), that would be excellent too.
Oh, and since this package is using Fedora trademarks, it must be licensed in the same manner that "fedora-release" is, with the same notices.
@Stephen: A couple of other nitpicks: * The ID should be "fedora-modular" rather than "fedoramodular". Likewise for the CPE name. * "ID_LIKE=fedora" should be added
Spec: https://sgallagh.fedorapeople.org/packagereview/modular-release/modular-release.spec SRPM: https://sgallagh.fedorapeople.org/packagereview/modular-release/fedora-modular-release-26-0.5.fc26.src.rpm (In reply to Neal Gompa from comment #12) > Oh, and since this package is using Fedora trademarks, it must be licensed > in the same manner that "fedora-release" is, with the same notices. I think it already is; did I miss something? (In reply to Neal Gompa from comment #13) > @Stephen: > > A couple of other nitpicks: > > * The ID should be "fedora-modular" rather than "fedoramodular". Likewise > for the CPE name. > * "ID_LIKE=fedora" should be added Done
@Stephen: * You've got the notice in place, so you're good there. * The spec should be called fedora-modular-release.spec * The package should be called fedora-modular-release * It should provide "system-modular-release" and "system-modular-release(%{version})" in addition to standard ones (c.f. edition release packages). - This allows for rebranding and generic versions to replace it easily enough. * The commented out "modular-repos(%{version})", if you decide to do that, should be "fedora-modular-repos(%{version})". Also, I don't see an updated spec posted. The SRPM seems to be updated, so maybe the spec is out of sync with the SRPM one.
Spec: https://sgallagh.fedorapeople.org/packagereview/modular-release/fedora-modular-release.spec SRPM: https://sgallagh.fedorapeople.org/packagereview/modular-release/fedora-modular-release-26-0.6.fc26.src.rpm (In reply to Neal Gompa from comment #15) > @Stephen: > > * You've got the notice in place, so you're good there. > > * The spec should be called fedora-modular-release.spec > > * The package should be called fedora-modular-release > This was fixed in the spec included in the SRPM, but I posted the wrong spec link above. > * It should provide "system-modular-release" and > "system-modular-release(%{version})" in addition to standard ones (c.f. > edition release packages). > - This allows for rebranding and generic versions to replace it easily > enough. > Done > * The commented out "modular-repos(%{version})", if you decide to do that, > should be "fedora-modular-repos(%{version})". > Done > Also, I don't see an updated spec posted. The SRPM seems to be updated, so > maybe the spec is out of sync with the SRPM one. Sorry, I pasted the wrong one into the comment. Fixed here.
@Stephen: Thanks for fixing URL for spec. Couple of remaining things I noticed: * URL is invalid. I think you mean https://github.com/sgallagher/modularity-release ? Or do you intend to have a Pagure repo for this? * NAME in os-release should be "Fedora Modular" rather than "Fedora". It should also be a quoted string (since it will have a space).
Spec: https://sgallagh.fedorapeople.org/packagereview/modular-release/fedora-modular-release.spec SRPM: https://sgallagh.fedorapeople.org/packagereview/modular-release/fedora-modular-release-26-0.7.fc26.src.rpm (In reply to Neal Gompa from comment #17) > @Stephen: Thanks for fixing URL for spec. > > Couple of remaining things I noticed: > > * URL is invalid. I think you mean > https://github.com/sgallagher/modularity-release ? Or do you intend to have > a Pagure repo for this? > Once it gets created in dist-git, we'll probably use dist-git as the official upstream source for it, since there's no benefit at all to having a separate upstream. For now, I've just fixed the URL (and renamed my github repo to match the new name). > * NAME in os-release should be "Fedora Modular" rather than "Fedora". It > should also be a quoted string (since it will have a space). Yes, thanks. I missed that. Good eye :)
Well, I'm not the official reviewer, but I'm happy now. :)
I reviwed the changes & am fine with them. Approving.