Bug 1279175

Hi,

As discussed by email I will review this and sponsor you as a Fedora packager when the review is done.

This is not yet a full review, but the result of a quick scan, which shows several issues:

1) Starting with the generic upstream spec file is fine but please drop:

1a) All the %if 0%{?suse_version} blocks (keeping the non suse code)

1b) The Group: and BuildRoot: lines we no longer use those

2) I do not think that keeping upstream's changelog in the spec file is is useful, please drop it and replace it with one with a single entry with you as author saying something like:

-Initial Fedora package based on upstream spec-file for 1.5-1

And make the release of the new version 2, so that your package version is 1.5-2, in Fedora we always bump the release field during reviews (and make changelog entries for any improvements) so that reviewers can easily track changes made during the review process.

3) rpmlint rpmbuild/SRPMS/innoextract-1.5-1.fc23.src.rpm rpmbuild/RPMS/x86_64/* results in :

innoextract.src: W: strange-permission innoextract-1.5.tar.gz 640
innoextract.src:54: W: mixed-use-of-spaces-and-tabs (spaces: line 18, tab: line 54)
innoextract.src: W: file-size-mismatch innoextract-1.5.tar.gz = 178011, http://constexpr.org/innoextract/files/innoextract-1.5.tar.gz = 179582
3 packages and 0 specfiles checked; 0 errors, 3 warnings.

The first message can be ignored, the second one needs to be fixed (please uses spaces everywhere) the third one is weird, please double check your sources match upstream.

Doing a diff on the 2 different tarbals unpacked says:

Only in innoextract-1.5.src.rpm: .mailmap
Only in innoextract-1.5.src.rpm: .travis.yml

So maybe upstream has respun the tarbal to remove these 2 unwanted files ?

Thanks & Regards,

Hans

1) ok

2) ok

I also keep an history of my changes on GitHub, is there a more appropriate location where to put this specfile,
or at least shouldn't this location be linked back from the specfile
in some machine-readable way ?

  https://github.com/a-detiste/fedora/commits/master

3) rpmlint:

- I've fixed the tab vs spaces.


- Didn't found a way to silence the spurious warning about "Inno" being a typo.
  "W: spelling-error %description -l en_US Inno -> In no, In-no, Inn"

  https://ask.fedoraproject.org/en/question/10738/rpmlint-override-in-spec-file/


- It's the tarball at GitHub, the one with the
  "Please do not use the automatically generated source archives below"
  disclaimer, mea culpa.


Thanks for the review

adetiste's scratch build of innoextract-1.5-2.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11908977

Hi Alexandre,

(In reply to Alexandre Detiste from comment #2)
>
> I also keep an history of my changes on GitHub, is there a more appropriate
> location where to put this specfile,

No, during review any place where you can provide links to the specfile and srpm is fine. Once the pkg passes review and becomes part of Fedora than Fedora's pkgs git is the canonical source for the specfile.

> or at least shouldn't this location be linked back from the specfile
> in some machine-readable way ?

Nope, all official spec files are kept at:
http://pkgs.fedoraproject.org/

So there is no need to include a link to the canonical source inside the specfile since there is only one canonical source for Fedora specfiles.

>   https://github.com/a-detiste/fedora/commits/master
> 
> 3) rpmlint:
> 
> - I've fixed the tab vs spaces.
> 
> 
> - Didn't found a way to silence the spurious warning about "Inno" being a
> typo.
>   "W: spelling-error %description -l en_US Inno -> In no, In-no, Inn"

Right, that is no problem, these messages can just be ignored.

> - It's the tarball at GitHub, the one with the
>   "Please do not use the automatically generated source archives below"
>   disclaimer, mea culpa.

You are using the right tarbal, the problem is that upstream seems to have replaced the tarbal (which is sorta a bad thing to do for upstream, but it happens) and your srpm has an old version inside, please replace it with the latest version from upstream.

(In reply to Upstream Release Monitoring from comment #3)
> adetiste's scratch build of innoextract-1.5-2.src.rpm for f23 completed
> http://koji.fedoraproject.org/koji/taskinfo?taskID=11908977

I assume this was a build with a spec file fixing all the issues mentioned before. When ever you do a new revision during pkg review please add a comment in bugzilla with links to a new srpm and new specfile.

I've done a review using the current: https://raw.githubusercontent.com/a-detiste/fedora/master/innoextract.spec for now, full review below:

Good:
====
- rpmlint checks return:
[hans@shalem rpmbuild]$ rpmlint RPMS/x86_64/innoextract-* SRPMS/*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
- package meets naming guidelines
- package meets packaging guidelines
- license (zlib) OK, text in %doc, matches source
- spec file legible, in am. english
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Needs work:
=======
- source matches upstream: Did not match for -1 srpm, no -2 srpm so could not check
- Please put a blank line between changelog entries, iow make the current changelog look like this:

* Thu Nov 19 2015 Alexandre Detiste <alexandre> - 1.5-2
- Remove "suse_version" blocks
- Drop Group: and BuildRoot: lines

* Sun Nov 08 2015 Alexandre Detiste <alexandre> - 1.5-1
- Initial Fedora package based on upstream spec-file for 1.5-1

If you can fix these 2 minor issues in a -3 version, then I will approve the package and once it is approved you can apply for membership to the packager group and I'll sponsor you.

Thanks & Regards,

Hans

Hi,

This time I didn't forgot to provide the SRPM, here it is:

https://github.com/a-detiste/fedora/blob/master/innoextract-1.5-3.src.rpm?raw=true

The specfile is still a the same location:

https://github.com/a-detiste/fedora/blob/master/innoextract.spec


Thanks for review

adetiste's scratch build of innoextract-1.5-3.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12341169

Hi,

(In reply to Alexandre Detiste from comment #5)
> Hi,
> 
> This time I didn't forgot to provide the SRPM, here it is:
> 
> https://github.com/a-detiste/fedora/blob/master/innoextract-1.5-3.src.
> rpm?raw=true

Looks good now: Approved!

I've gone ahead and added you to the packagers group in FAS and sponsored you.

Regards,

Hans

Hi Alexandre,

In case it was not clear, you can proceed with this step now:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Regards,

Hans

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/innoextract

https://fedoraproject.org/wiki/Package_Review_Process #15 
told me to do that

Hi Alexandre,

I just noticed that your .spec has:

Release:   3

instead of:

Release:   3%{?dist}

This is against the guidelines, and more importantly causes problems when sharing the spec between releases (which is advisable). Can you please bump the release to 4%{?dist}, add a matching changelog entry and then build ?

This will give you a: innoextract-1.5-4.fc24 build.

Usually new packages are also introduced in currently supported releases in Fedora, I believe it would be good to bring innoextract to at least Fedora 23, to do this switch to the f23 branch, merge it with master and then push and build:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Update_Your_Branches_.28if_desired.29

This will give you a innoextract-1.5-4.fc23 build, this is where the %{?dist} comes into play, the 2 release fields differing will ensure that users get the f24 build (which may use a different compiler, lib versions etc) when upgrading to 24.

Once you've done a f23 build, it will not appear in the repos automatically, you will need to create an update for it as described here:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Submit_Package_as_Update_in_Bodhi

Also see:

https://fedoraproject.org/wiki/Package_update_HOWTO#Later_Branched_and_stable_releases

Regards,

Hans

> %{?dist}

I First though it was there for SuSE support; the noticed it in Fedora doc but forgot to put it back.

> This will give you a: innoextract-1.5-4.fc24 build.

Upstream told me today he wanted to make a new release that would support the latest InnoSteup format; so that would be "innoextract-1.6-1.fc24".


> (backport)

Backports should be easy, Upstream provides packages back to Fedora 20 in his private repository:

https://build.opensuse.org/package/binaries/home:dscharrer/innoextract?repository=Fedora_23

Regards,

Alexandre

The f23 branch doesn't exist.


And in https://fedoraproject.org/wiki/Package_maintenance_guide ;
linked wiki page "adding a new release branch" to an existing package
doesn't exist.



[tchet@fedora innoextract]$ fedpkg switch-branch f23
Could not execute switch_branch: Unknown remote branch origin/f23
[tchet@fedora innoextract]$ git pull --all
Récupération de origin
WARNING: 'innoextract' is an alias for 'rpms/innoextract'
Already up-to-date.
[tchet@fedora innoextract]$ fedpkg switch-branch f23
Could not execute switch_branch: Unknown remote branch origin/f23
[tchet@fedora innoextract]$ git remote
origin
[tchet@fedora innoextract]$ git branch f23
[tchet@fedora innoextract]$ fedpkg switch-branch f23
Switched to branch 'f23'
[tchet@fedora innoextract]$ git merge master
Already up-to-date.
[tchet@fedora innoextract]$ git push
fatal: La branche courante f23 n'a pas de branche amont.
Pour pousser la branche courante et définir la distante comme amont, utilisez

    git push --set-upstream origin f23

[tchet@fedora innoextract]$  git push --set-upstream origin f23
WARNING: 'innoextract' is an alias for 'rpms/innoextract'
Total 0 (delta 0), reused 0 (delta 0)
remote: FATAL: C refs/heads/f23 rpms/innoextract adetiste DENIED by refs/heads/f[0-9][0-9]
remote: error: hook declined to update refs/heads/f23
To ssh://adetiste.org/innoextract
 ! [remote rejected] f23 -> f23 (hook declined)
error: impossible de pousser des références vers 'ssh://adetiste.org/innoextract'

Found:

https://fedoraproject.org/wiki/PackageDB_admin_requests#Additional_branches_for_existing_packages

I fixed the link on the Wiki

You dont search before submit :)

https://bugzilla.redhat.com/show_bug.cgi?id=1239267