Bug 757156 - Review Request: perl-Env-C - Get/Set/Unset Environment Variables on the C level [NEEDINFO]
Review Request: perl-Env-C - Get/Set/Unset Environment Variables on the C level
Status: CLOSED INSUFFICIENT_DATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul Howarth
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-25 10:35 EST by Jan "Yenya" Kasprzak
Modified: 2015-04-21 05:18 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-02-05 03:25:10 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
paul: fedora‑review?
paul: needinfo? (kas)


Attachments (Terms of Use)

  None (edit)
Description Jan "Yenya" Kasprzak 2011-11-25 10:35:37 EST
Spec URL: http://www.fi.muni.cz/~kas/tmp/perl-Env-C.spec
SRPM URL: http://www.fi.muni.cz/~kas/tmp/perl-Env-C-0.08-1.fc16.src.rpm
Description:
This module provides a Perl API for getenv(3), setenv(3) and unsetenv(3).
It also can return all the environ variables.
Review Description: this is my first CPAN package ported to Fedora RPM, so I will need a sponsor.
Comment 1 Willington Vega 2011-12-02 08:50:51 EST
Hello Jan,

I'm currently learning about Fedora package guidelines so I can't sponsor you, but I would like to provide an informal review for your package.

rpmlint return no errors for your SPEC:
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint return some warnings for the SRPM you provided and generated RPMs packages, but I don't think that's a problem:
perl-Env-C.src: W: spelling-error %description -l en_US getenv -> Genet
perl-Env-C.src: W: spelling-error %description -l en_US setenv -> seventeen
perl-Env-C.src: W: spelling-error %description -l en_US unsetenv -> unsweetened
perl-Env-C.src: W: spelling-error %description -l en_US environ -> environs, environment
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

The package builds also builds in mock.

Also, the spec file includes the following when listing directory ownership:

%{perl_vendorarch}/auto/*
%{perl_vendorarch}/Env*

but the Perl packaging guidelines[1] says it should be:

# For arch-specific packages: vendorarch
%{perl_vendorarch}/*
%exclude %dir %{perl_vendorarch}/auto/

will that work for this package?

Also, I went through the MUST and SHOULD[2] and couldn't find any blockers. One of the SHOULDs I want to note is that there is no license text as separated file in the sources.

Hope this helps to get more attention to and improve your package.

1. http://fedoraproject.org/wiki/Packaging:Perl
2. https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
Comment 2 Willington Vega 2011-12-02 08:59:34 EST
Perhaps is a good idea to write to Fedora Perl SIG mailing list[1] and let them know about your package. Not sure if you already this.

1. https://lists.fedoraproject.org/mailman/listinfo/Perl-devel
Comment 3 Jan "Yenya" Kasprzak 2011-12-02 10:18:00 EST
Re: comment #1

Hello Willington, thanks for the review!

I think spelling errors reported by rpmlint are not valid. I have updated the .spec and src.rpm file to fix the directory ownership problem you have reported. It would be nice to have cpanspec modified so that it generated a .spec file which actually follows the guidelines. I will report the issue to the cpanspec package.

As for the license text: perl modules often refer to the license of Perl itself (dual GPL+Artistic). This one explicitly states only Artistic license (which makes sense for smallish modules like this one).

Re: comment #2

I have joined the list and let them know about this package.
Comment 4 Willington Vega 2011-12-03 09:12:38 EST
Hi Jan,

I agree, those spelling errors are not a problem and reporting the issue with the directory ownership back to the cpanspec package is a good idea.

I've seen on other reviews that is good to provide the updated spec file and srpm as a different one, with the modifications, an increased release number and the corresponding Changelog documentation. It is also helpful when comparing spec files to debug errors like the one I'm going to describe next:

The package no longer builds in mock from the SRPM:
mock SRPMS/perl-Env-C-0.08-1.fc15.src.rpm

The important part of the build.log file says:

+ make test
PERL_DL_NONLAZY=1 /usr/bin/perl "-Iblib/lib" "-Iblib/arch" test.pl
Can't locate Test/More.pm in @INC (@INC contains: blib/lib blib/arch /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5 .) at test.pl line 4.
BEGIN failed--compilation aborted at test.pl line 4.
make: *** [test_dynamic] Error 2
RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.yd2GUq (%check)
    Bad exit status from /var/tmp/rpm-tmp.yd2GUq (%check)
Child returncode was: 1

The problem is your updated spec file no longer BuildRequires perl(Test::More).

Thank you for your quick update.
Comment 5 Paul Howarth 2011-12-06 10:29:57 EST
I'm concerned about the license of this package: upstream just refers to "Artistic" but your spec says "Artistic 2.0". The latter would be acceptable for Fedora but the former isn't. Could you get upstream to clarify which version of the Artistic license they mean? Some upstreams have been known to say "Artistic" when they actually mean "same as Perl", which might possibly be the case here too.

Have you submitted any other packages for review yet?
Comment 6 Jan "Yenya" Kasprzak 2011-12-06 12:06:20 EST
Re: comment #4

my fault, I have added dependency on Test::More back. I have also bumped the release number and added a %changelog entry as you suggest:

http://www.fi.muni.cz/~kas/tmp/perl-Env-C.spec
http://www.fi.muni.cz/~kas/tmp/perl-Env-C-0.08-2.fc16.src.rpm

Re: comment #5

I have asked upstream to clarify the licensing.

No, I have not submitted any other packages. Should I submit them? I wanted to go through the whole process of making a Fedora package with the single package first.

If anybody is interested in (pre-)review, I have uploaded them to http://www.fi.muni.cz/~kas/tmp/fedora-packages/. Some of them cannot be built in mock as-is, because they depend on other packages from this set, so they have probably to be reviewed in a single request. Also, they all curretly have the problem mentioned in comment #1 (directory ownership of %{perl_vendorarch}... as created by cpanspec).
Comment 7 Paul Howarth 2011-12-06 13:58:20 EST
(In reply to comment #6)
> Re: comment #5
> 
> I have asked upstream to clarify the licensing.

Good.

> No, I have not submitted any other packages. Should I submit them? I wanted to
> go through the whole process of making a Fedora package with the single package
> first.

The whole process can't continue much further with this one until the license is clarified unfortunately, which is why it would be a good idea to submit a few others too.

Regarding your plans for participation in Fedora, do you see yourself sticking to perl modules or might you have other things to bring in in the future?

> If anybody is interested in (pre-)review, I have uploaded them to
> http://www.fi.muni.cz/~kas/tmp/fedora-packages/. Some of them cannot be built
> in mock as-is, because they depend on other packages from this set, so they
> have probably to be reviewed in a single request.

Are there any more "leaf" packages, that don't have any non-Fedora dependencies?

> Also, they all currently have
> the problem mentioned in comment #1 (directory ownership of
> %{perl_vendorarch}... as created by cpanspec).

This is more a matter of taste. If I was packaging this module, I'd use:

%files
%defattr(-,root,root,-)
%doc Changes README
%{perl_vendorarch}/auto/Env/
%{perl_vendorarch}/Env/
%{_mandir}/man3/Env::C.3pm*

I prefer to keep wildcards to a minimum so that if future versions start shipping different files, I notice it and can investigate to see if it's a problem or not. What's important is that your package "owns" the necessary directories and files. All versions of this package that you have submitted thus far satisfy that requirement.
Comment 8 Jan "Yenya" Kasprzak 2011-12-07 15:34:19 EST
OK, here is the quoted discussion with the upstream package author. Lines prefixed with ">" are mine, and lines prefixed with ":" are written by the package author:

: > : Personally, I don't really care which exact of the free-as-a-beer
: > : license is used, GPL, Artistic XYZ, or whatever you'd like it to be.
: >
: >       OK. So "the same terms as Perl itself" would be OK with you?
: [...]
: >       So if you say "the same terms as Perl itself" or "Artistic 2.0"
: > or "Artistic clarified" is OK with you, I would be able to package
: > Env::C for Fedora.
: 
: Yes, either of the above works. I doubt I'll release a new version of
: the module just to tweak this, unless it's really important. Perhaps
: you can just quote this communication as a proof.

Is it sufficient to have it this way and keep License: Artistic 2.0
in the spec file?
Comment 9 Jan "Yenya" Kasprzak 2011-12-07 15:53:41 EST
Re: comment #7
> Regarding your plans for participation in Fedora, do you see yourself sticking
> to perl modules or might you have other things to bring in in the future?

I want to contribute packages which I need myself, but recently everything I have needed was already packaged except some Perl modules. So for now, I will stick with Perl modules. (context: I have been building RPM packages for at least 12 years now, does anybody here remember Red Hat Contrib|Net, the early community packaging effort for Red Hat Linux?).

> Are there any more "leaf" packages, that don't have any non-Fedora
> dependencies?

E.g. perl-TeX-Encode, perl-Scalar-String, perl-IO-Socket-Multicast, perl-Data-Integer, perl-Data-Float, or perl-DBD-ODBC.

> > Also, they all currently have
> > the problem mentioned in comment #1 (directory ownership of
> > %{perl_vendorarch}... as created by cpanspec).
> 
> This is more a matter of taste.

Well, perhaps the Packaging:Perl wiki page should be modified to not have this requirements then? (I cannot check the exact wording as Fedora wiki currently says 503 Service Unavailable for me).
Comment 10 Paul Howarth 2011-12-08 06:56:24 EST
(In reply to comment #8)
> OK, here is the quoted discussion with the upstream package author. Lines
> prefixed with ">" are mine, and lines prefixed with ":" are written by the
> package author:
> 
> : > : Personally, I don't really care which exact of the free-as-a-beer
> : > : license is used, GPL, Artistic XYZ, or whatever you'd like it to be.
> : >
> : >       OK. So "the same terms as Perl itself" would be OK with you?
> : [...]
> : >       So if you say "the same terms as Perl itself" or "Artistic 2.0"
> : > or "Artistic clarified" is OK with you, I would be able to package
> : > Env::C for Fedora.
> : 
> : Yes, either of the above works. I doubt I'll release a new version of
> : the module just to tweak this, unless it's really important. Perhaps
> : you can just quote this communication as a proof.
> 
> Is it sufficient to have it this way and keep License: Artistic 2.0
> in the spec file?

Yes, but include the email from the author saying this is OK as one of the %doc files in the package. That's what happened a while back for perl-NetAddr-IP, which had the same issue.

http://pkgs.fedoraproject.org/gitweb/?p=perl-NetAddr-IP.git;a=blob;f=License_of_perl-NetAddr-IP.txt;h=661544743aa94efa0c8e55501ffb6a755c5dd921;hb=b803a15fae9080ce0fd35bbdaab73fd168d0d275

(In reply to comment #9)
> Re: comment #7
> > Regarding your plans for participation in Fedora, do you see yourself sticking
> > to perl modules or might you have other things to bring in in the future?
> 
> I want to contribute packages which I need myself, but recently everything I
> have needed was already packaged except some Perl modules. So for now, I will
> stick with Perl modules. (context: I have been building RPM packages for at
> least 12 years now, does anybody here remember Red Hat Contrib|Net, the early
> community packaging effort for Red Hat Linux?).

The issue here is that perl module packages tend to be very formulaic and generally don't need much tweaking from what you get out of cpanspec. As a result, package submissions of perl modules don't provide much insight into your understanding of packaging in general or the Fedora guidelines in particular. The usual approach to demonstrating this understanding is to do unofficial reviews of other packages like Willington did for this one (though preferably not just perl modules).

> > Are there any more "leaf" packages, that don't have any non-Fedora
> > dependencies?
> 
> E.g. perl-TeX-Encode, perl-Scalar-String, perl-IO-Socket-Multicast,
> perl-Data-Integer, perl-Data-Float, or perl-DBD-ODBC.

If there's any of those with anything "unusual" about them, submit that, else pick one at random for now.

> > > Also, they all currently have
> > > the problem mentioned in comment #1 (directory ownership of
> > > %{perl_vendorarch}... as created by cpanspec).
> > 
> > This is more a matter of taste.
> 
> Well, perhaps the Packaging:Perl wiki page should be modified to not have this
> requirements then? (I cannot check the exact wording as Fedora wiki currently
> says 503 Service Unavailable for me).

Maybe the wording could be clearer. The guideline is talking about which directories must be owned by the package, by way of some sample %files entries. The important bit is the directory ownership rather than the implementation though.
Comment 11 Jan "Yenya" Kasprzak 2011-12-23 07:58:39 EST
OK, I have created a new .spec and .src.rpm file. The only difference is added e-mail from upstream in %doc:

http://www.fi.muni.cz/~kas/tmp/perl-Env-C-0.08-3.src.rpm
http://www.fi.muni.cz/~kas/tmp/perl-Env-C.spec
Comment 12 Jan "Yenya" Kasprzak 2012-02-17 08:45:16 EST
New Package SCM Request
=======================
Package Name: perl-Env-C
Short Description: Get/Set/Unset Environment Variables on the C level
Owners: yenya
Branches: f15 f16 f17
InitialCC:
Comment 13 Jon Ciesla 2012-02-17 08:45:30 EST
Please include an SCM request.
Comment 14 Jan "Yenya" Kasprzak 2012-02-17 10:10:13 EST
New Package SCM Request
=======================
Package Name: perl-Env-C
Short Description: Get/Set/Unset Environment Variables on the C level
Owners: yenya
Branches: f15 f16 f17
InitialCC: perl-sig
Comment 15 Jon Ciesla 2012-02-17 10:30:35 EST
In looking closer, it doesn't look like this has been approved.
Comment 16 Jan "Yenya" Kasprzak 2012-02-17 11:29:35 EST
Oh, well. I have received an e-mail that I have been sponsored, and the following text:

> Thank you for applying for the packager group.
> 
> Welcome to the Fedora packager group. Please continue the process from:
> https://fedoraproject.org/wiki/PackageMaintainers
> /Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Which is what I have been trying to follow today. If this is not the correct procedure, anybody care to enlighten me what am I missing? Thanks!
Comment 17 Petr Šabata 2012-02-20 04:18:21 EST
It seems like Paul was working on the review here.  He should assign the bug to himself and when he's happy with your package, set the 'fedora-review+' flag.  Then you can submit the SCM request with 'fedora-cvs?' again.
Comment 18 Paul Howarth 2012-05-23 09:24:37 EDT
(In reply to comment #16)
> Oh, well. I have received an e-mail that I have been sponsored, and the
> following text:
> 
> > Thank you for applying for the packager group.
> > 
> > Welcome to the Fedora packager group. Please continue the process from:
> > https://fedoraproject.org/wiki/PackageMaintainers
> > /Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
> 
> Which is what I have been trying to follow today. If this is not the correct
> procedure, anybody care to enlighten me what am I missing? Thanks!

I didn't know that Radek Vokál had sponsored you, and you didn't remove the FE-NEEDSPONSOR blocker from this ticket (which I've now done). Given that you're now sponsored, we should be able to finish this review off quickly.

Your SRPM seems to have disappeared; can you put it back so I can check it out? You might also want to remove the "find" command that removes empty directories from the buildroot:

  find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;

This isn't actually needed as rpm-build ignores empty directories (though not empty files) in the buildroot.
Comment 19 Paul Howarth 2012-08-28 07:16:03 EDT
Jan, are you still interested in this package?
Comment 20 Paul Howarth 2013-08-19 04:58:31 EDT
This review appears to be stalled; please respond soon if you're still interested in submitting this package.
Comment 21 Paul Howarth 2015-02-05 03:25:10 EST
Closing due to lack of response. Please re-open if still interested.

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