Bug 240807 - Review Request: asl - Macro Assembler AS
Review Request: asl - Macro Assembler AS
Status: CLOSED DUPLICATE of bug 581334
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Xavier Lamien
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-21 17:04 EDT by Eric Smith
Modified: 2010-04-11 19:29 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-23 13:30:42 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lxtnow: fedora‑review-


Attachments (Terms of Use)

  None (edit)
Description Eric Smith 2007-05-21 17:04:19 EDT
Spec URL: http://www.brouhaha.com/~eric/software/fedora/fc6/asl/asl.spec
SRPM URL: http://www.brouhaha.com/~eric/software/fedora/fc6/asl/asl-1.42-0.8.bld55.src.rpm
Description: 
AS is a portable macro cross-assembler for a variety of microprocessors
and controllers. Although it is mainly targeted at embedded processors
and single-board computers, you also find CPU families that are used in
workstations and PCs in the target list.
Comment 1 Xavier Lamien 2007-06-01 22:55:56 EDT
well,

-----------
Spec file:
-----------

your spec file doesn't follow packaging guidelines and SHOULD be fix.


* release tag : isn't good.
                as it's your first build for this package the release MUST be   
                set to 0.1 (as it's provided with build version release).
                So, the release should be 0.1.build55 instead of 0.8.bld55.


* Source tag : the source path should be point to externel downloadable link or 
               comment why you can't paste the full location in spec file.


* Buildroot tag : should be review, see packaging guidelines page.


* %build stage : mock's working...comment is coming soon.
                 About "make test" command: it's SHOULD be paste in
                 %check stage juste add it after %build stage.


* %clean stage is missing, SHOULD be fix.


* %files stage : this section can be improved.
                 the use of %dir /path/to/folder is not needed to own package.
                 So your %files section will looks like this:

%files
%defattr(-,root,root)
%doc COPYING README* TODO BENCHES changelog
%doc doc/*
%{_bindir}/*
%{_datadir}/asl
%{_datadir]/asl/include
%{_datadir}/asl/lib
%{_mandir}/man1/*.gz


* note that I added changelog file which was missing.
* also note the use of macros instead of hardcoded path (which's not apropriate)


Comment 2 Eric Smith 2007-06-02 01:42:59 EDT
Thank you for the review.  I will make the changes you have requested.

Why should the release be "0.1build55" rather than "0.1bld55"?  The author names
the source file and changelog entries with "bld" and "Bld", but not "build".

It is not clear to me where the naming guidelines require me to submit the
package for review with the release number of 1. The guidelines say:

    The release number (referred to in some older documentation as a "vepoch")
is how the maintainer marks build revisions, starting from 1.

I have followed that guideline, and the package I submitted for review was the
eighth build revision.

I'm not disputing whether the build revision of the package initially submitted
for review should be 1, bur rather suggesting that the documentation is not
clear on this point, and perhaps it should be made more explicit.

When I submit my next build revision, I'll set it to 1, though I dislike the
loss of build traceability.

Eric
Comment 3 Xavier Lamien 2007-06-02 02:23:30 EDT
well,

the release 0.8 is the release which came from your rpm build release not from
upstream.

The changelog which included in the source tarball show that the current release
is 1.42-bld55 and not 1.42.8bld55 or 1.42.0.8bld55.
So your source tarball release is build55 and not 0.8.

why you should set it to 0.1 instead of 1 ?
Cause you have a release (bld55) from your upstream and in this case the rpm
build release is not anymore 1 but 0.1 to lead the release source tarball (just
like packages with beta5, pre2 or rc3 release).

Comment 4 Eric Smith 2007-06-02 18:21:00 EDT
I don't think you understood my questions; perhaps I didn't explain clearly
enough, so I'll try again.

I understand the reason it is 0.nsomething.

My first question was why you wanted the "something" part to be build55 rather
than bld55.  The upstream designation is bld55, not build55.

My second question was why you wanted the n in 0.n to be 1.  I started with 1
(0.1bld55), and the SRPM I submitted for review was my eighth build release, so
the n had been incremented to 8, thus 0.8bld55.  This seems consistent with the
packaging and naming guidelines, which state that the packager is supposed to
increment the build release for each build change from the same upstream
package; that's exactly what I've done.

The packaging and naming guidelines do NOT anywhere say that the build release
number should be reverted to 1 when the package is submitted for review.  If
this is really the correct and intended process, I'm willing to do it, but it
should be documented as such.

Thanks,
Eric
Comment 5 Xavier Lamien 2007-06-03 11:33:00 EDT
:-)

>> My first question was why you wanted the "something" part to be build55 rather
>> than bld55.  The upstream designation is bld55, not build55.

it's just a packaging naming question for fedora packages names.
i.e: a pacakge named bar-1.2-b5.tar.gz will be named for fedora inclusion like
that -> bar-1.2-0.1.beta5.%{dist}.%{arch}.rpm
Same thing for bld which'll become build.



>> My second question was why you wanted the n in 0.n to be 1.  I started with 1
>> (0.1bld55), and the SRPM I submitted for review was my eighth build release,
>> so the n had been incremented to 8, thus 0.8bld55.

If so, your Changelog section is not correct.
Each build/fix you've done from your spec/pakcage files SHOULD be mentioned in
the changelog section.
As it's your eighth build release, your seven first SHOULD precede this last one
in spec file.
When you update or fix some features in your package you have to increment your
release to n+1 and add lines (not replace the old) in changelog about this new
build.



Comment 6 Eric Smith 2007-06-03 18:36:41 EDT
Thanks for the further clarifications.  Since there was a discrepancy between
the build release and changelog history, I'll use 1 for the build release for
the next spec as you've requested.

After re-reading the "Non-Numeric Version in Release" portion of the
Packaging/Naming Guidelines, I can't find anything suggesting that "bld" needs
to be expanded to "build":

   %{alphatag} is the string that came from the version.

In this case, "the string that came from the version" is clearly "bld".  I don't
see any documentation suggesting that the alphatag should be converted into a
canonical form.

However, it does say that there should be a dot between my build number and the
alpha tag, so it appears that I should submit a packaged named
asl-1.42-0.1.bld55  If there is documentation supporting replacing "bld" with
"build", please provide a reference, and I'll do it.

Thanks!
Eric
Comment 7 Eric Smith 2007-06-03 18:43:28 EDT
Ignore my comments about the dot, I got that right before, and it was my own
omission (typo) in the bug discussion that dropped it.

Also, I am not going to use "*" in the files section, because that would prevent
RPM from notifying the packager of new files in new releases.  I assume that you
used "*" in your suggested change to simplify the exposition, rather than really
wanting "*" in the actual spec.  I will, however, use the path macros.

Eric
Comment 8 Eric Smith 2007-06-03 19:16:57 EDT
Here is the updated package:

Spec URL: http://www.brouhaha.com/~eric/software/fedora/fc6/asl/asl.spec
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/fc6/asl/asl-1.42-0.1.bld55.src.rpm

The SRPMs and RPMs pass rpmlint with no warnings or errors.  The package builds
with mock on FC6 for both i386 and x86_64.

I believe I have addressed all of the issues you raised other than the "bld" vs
"build", and I'll change that too if supporting documentation is provided.

Thanks!
Eric
Comment 9 Xavier Lamien 2007-06-04 01:54:37 EDT
Okay,
I will check this out.

But, you should really take in account my comment about ownership files in
package from [comment #1]

The %dir macros is more/better apropriate to own an empty directory which is
mandatory to be added in package.
Comment 10 Eric Smith 2007-06-04 02:47:05 EDT
Right.  I've now added the relevant %dir directives, and the use of smp_mflags.

Spec URL: http://www.brouhaha.com/~eric/software/fedora/fc6/asl/asl.spec
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/fc6/asl/asl-1.42-0.2.bld55.src.rpm

Thanks!
Eric
Comment 11 Xavier Lamien 2007-06-06 00:49:20 EDT
>>  I've now added the relevant %dir directives, and the use of smp_mflags.

i meant that you should remove %dir directive, it's not mandatory to own directory.
you should only use %{_datadir}/%{name} instead.
All files/sub-directory its contains will be included too.
That better and improve update from next release.
Comment 12 Eric Smith 2007-06-06 02:12:39 EDT
In my opinion, it is better to use an explicit list of files in the files
section, in order to get an unpackaged files warning if new files are added in a
future upstream release.  This does not seem to violate the packaging guidelines.

However, since you seem to feel very strongly about it, rather than debate it
further I have changed it as you have requested:

Spec URL: http://www.brouhaha.com/~eric/software/fedora/fc6/asl/asl.spec
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/fc6/asl/asl-1.42-0.3.bld55.fc6.src.rpm
Comment 13 Mamoru TASAKA 2007-06-06 04:04:06 EDT
(REMOVING NEEDSPONSOR. I will sponsor the submitter
 bug 241099)
Comment 14 Xavier Lamien 2007-06-06 22:40:04 EDT
It's a good things that packager check changes from his maintained packages.
but if there is changes or something wrong from %files don't worry mock/rpmbuild
will show you.

just quick note:
%docs directive isn't necessary for man files.
%{_mandir}/man1/<files>  -> is enough

well,

OK - Mock : Built on F-7 (x86_64)
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License field in spec matches
OK - License is GPL
OK - License match extras packaging policy licenses allowed
OK - License file is included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources SHOULD match upstream md5sum:
c578c07675431f40c514921db6fdd102  asl-current-142-bld55.tar.bz2
OK - Package has correct buildroot.
OK - BuildRequires is not redundant for this package.
OK - %build and %install stages is correct and work.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Changelog section is correct.

OK - Should function as described.
OK - Should package latest version

------------------------------------------------
Rpmlint output:
------------------------------------------------
OK - silent on both srpm and rpm.



Comment 15 Xavier Lamien 2007-06-13 21:10:01 EDT
ping ?
Comment 16 Eric Smith 2007-06-13 21:26:18 EDT
Still here, but events have conspired to keep me very busy.  I'll try to get
another update to this completed this weekend, and ready to check into CVS.

Thanks,
Eric
Comment 17 Xavier Lamien 2007-07-16 19:59:36 EDT
I will close this bug as NOTABUG if no response is
received within ONE WEEK.

Comment 18 Xavier Lamien 2007-07-23 13:30:42 EDT
CLOSING.

If someone want to maintain this package, please sumbit a new
review request, Thanks.

Comment 19 Eric Smith 2010-04-11 19:29:30 EDT

*** This bug has been marked as a duplicate of bug 581334 ***

Note You need to log in before you can comment on or make changes to this bug.