Bug 225758 - Merge Review: flex
Merge Review: flex
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:38 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version: 2.5.33-9.fc7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-25 19:25:08 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:38:17 EST
Fedora Merge Review: flex

http://cvs.fedora.redhat.com/viewcvs/devel/flex/
Initial Owner: pmachata@redhat.com
Comment 1 Ralf Corsepius 2007-02-01 22:18:02 EST
MUSTFIX:

- package must not own
/usr/share/locale/*
and
/usr/share/locale/*/LC_MESSAGES

- spec should use %find_lang
Comment 2 Petr Machata 2007-02-02 08:49:57 EST
Fixed in rawhide.

Btw, rpmlint output:
W: flex devel-file-in-non-devel-package /usr/lib/libfl.a
W: flex devel-file-in-non-devel-package /usr/include/FlexLexer.h

flex is itself a development package, thus FlexLexer.h is OK here. libfl.a has
to be static for the same reason, so that scanners that are linked against it
don't runtime-depend on flex.
Comment 3 Ralf Corsepius 2007-02-02 10:22:36 EST
(In reply to comment #2)
> Fixed in rawhide.
Hmm? Either you forgot to commit your, or the public cvs server is a different
one the one you are accessing (and has not sync'ed yet).

It is NOT fixed in 
cvs.fedora.redhat.com:/cvs/dist/devel/flex

> Btw, rpmlint output:
> W: flex devel-file-in-non-devel-package /usr/lib/libfl.a
> W: flex devel-file-in-non-devel-package /usr/include/FlexLexer.h
That's OK with me.
Comment 4 Petr Machata 2007-02-02 11:18:24 EST
Aha, the CVS that's accessible from outside is a read-only copy of Red Hat's
CVS. I don't know how often do they sync.
Comment 5 Petr Machata 2007-05-24 13:28:11 EDT
The above indicates the package passed the review.  Closing.
Comment 6 Jason Tibbitts 2007-05-24 13:55:31 EDT
I don't see an APPROVED or the fedora-review flag set to '+'.  Ralf, were you
actually reviewing this?  If not, I can do a quick runthrough.
Comment 7 Ralf Corsepius 2007-05-25 10:00:28 EDT
(In reply to comment #6)
> I don't see an APPROVED or the fedora-review flag set to '+'.  Ralf, were you
> actually reviewing this?
Technically yes, formally no.
Comment 8 Petr Machata 2007-05-25 16:02:35 EDT
Sorry, my bad.
Comment 9 Jason Tibbitts 2007-05-26 01:28:36 EDT
OK, this package is pretty much OK but I just want to run the static library by
FESCo so that these merge reviews get the same treatment that any other package
would get.  Some other comments:

Source: should be a URL, probably
  http://dl.sf.net/flex/flex-%{version}.tar.bz2

This package has a build-time dependency on "info" but I don't see why a
text-mode info browser would be useful for the build process.

I'll get back as soon as FESCo has a chance to discuss the static library issue.

* source files match upstream:
   53b56a62ea9409b99b7a0ac4a5204fac16ca7eaf39b9374164c346d6badc6914  
   flex-2.5.33.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
? Not sure what BuildRequires: info is for.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
O rpmlint warnings are OK (static library OK pending FESCo ack).
* final provides and requires are sane:
   flex = 2.5.33-7.fc7
  =
   /bin/sh
   /sbin/install-info
   m4
* %check is present and all tests pass:
   Tests succeeded: 40
   Tests FAILED: 0
* no shared libraries are added to the regular linker search paths.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets are OK (install-info)
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers present, in base package because this package is only useful for 
  development.
* no pkgconfig files.
? static libraries present; FESCo ack pending.
* no libtool .la droppings.
Comment 10 Ralf Corsepius 2007-05-26 01:48:46 EDT
* compiler flags are appropriate.
gcc ... -I/usr/include ...

This -I/usr/include is a bug.
Comment 11 Jason Tibbitts 2007-05-26 01:57:51 EDT
You hadn't mentioned that before.  Please explain for the record, thanks.
Comment 12 Ralf Corsepius 2007-05-26 02:18:46 EDT
Done so many times before ;)

1. /usr/include the system header directory and is on a compiler's internal
include file search path. It should never be explicitly passed. Packages
requiring -I/usr/include are broken "by definition" and are likely to be badly
designed.

2. In this particular case, the culprit is toplevel Makefile.am appending
@includedir@ to AM_CPPFLAGS:

AM_CPPFLAGS = ... -I@includedir@ 

This is abuse of the autotools - @includedir@ is a destination directory (a
directory where files are supposed to be installed to).

Comment 13 Jason Tibbitts 2007-05-26 12:48:01 EDT
But, pedantry aside, can you point to any problems that this causes which are
worse than the possible fixes (of patching it out of Makefile.am and running the
autotools or patching it out of the generated Makefile)?  Because in the absense
of any actual breakage, I'm inclined to simply suggest that this issue be
reported to upstream for fixing there.
Comment 14 Ralf Corsepius 2007-05-27 04:52:59 EDT
(In reply to comment #13)
> But, pedantry aside, 
Pedantry? This is a stupid upstream bug, this package's maintainer (@RH) should
contact upstream to have it fixed.

> can you point to any problems that this causes which are
> worse than the possible fixes
Install an alternative GCC to /usr/local, cross-build the package and building
this package with go up in smoke.

> (of patching it out of Makefile.am and running the
> autotools or patching it out of the generated Makefile)?  Because in the absense
> of any actual breakage, I'm inclined to simply suggest that this issue be
> reported to upstream for fixing there.
Right, it doesn't affect building the rpm inside of the buildsystem. But it can
affect users rebuilding the package.

Comment 15 Jason Tibbitts 2007-05-27 12:39:31 EDT
(In reply to comment #14)
> Pedantry?

Yes, indeed, pedantry.

> This is a stupid upstream bug, this package's maintainer (@RH) should
> contact upstream to have it fixed.

Gee, that's exactly what I wrote.  Thanks for backing me up!

I'll approve this package once FESCo acks the static library issue.
Comment 16 Jason Tibbitts 2007-05-30 00:13:05 EDT
After some disussion with other FPC folks, I realized that recent changes to the
static library packaging guidelines require only that linking against static
libraries receive a FESCo ack.  Which means we'll have to look at getting some
sort of blanket exception for flex-using applications, but which means that this
package is fine.

One additional thing I noticed which should be fixed: the package isn't built
with parallel make.  Is there a reason why it isn't?  I built on an 8-way
machine with parallel make turned on and it went fine; the build completed and
the test suite passed with no problems.  But if there is a reason why parallel
make isn't supported, it needs to be commented in the spec.

Really, that's all; I'll trust you to fix up the make bit.

APPROVED
Comment 17 Ralf Corsepius 2007-05-30 01:39:36 EDT
(In reply to comment #16)
> After some disussion with other FPC folks, I realized that recent changes to the
> static library packaging guidelines require only that linking against static
> libraries receive a FESCo ack.  Which means we'll have to look at getting some
> sort of blanket exception for flex-using applications,
You should understand that flex package is devel package and must contain a
libfl* to work correctly.

However, in libfl* is such kind of trivial, that it hasn't seen any changes for
years and is very unlikely to see any changes ever. Furthermore, libfl is almost
never used by real world applications, because they typically replace it.

I.e. the correct way to ship flex is to package "a libfl*" into flex. If it's
libfl.a, so be it.
Comment 18 Petr Machata 2007-06-25 10:50:14 EDT
I cleaned up the spec per comments.  New version was built and the push is
pending.  I checked that upstream doesn't have the @include@ problem, perhaps it
was reported by someone already.
Comment 19 Fedora Update System 2007-06-25 19:25:01 EDT
flex-2.5.33-9.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.

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