Bug 226299 - Merge Review: pkgconfig
Merge Review: pkgconfig
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Package Reviews List
:
Depends On: 224148 550470
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 15:40 EST by Nobody's working on this, feel free to take it
Modified: 2010-08-18 16:27 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-08-18 16:27:01 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
susi.lehtola: fedora‑review+


Attachments (Terms of Use)
Patch against rawhide spec (1.07 KB, patch)
2010-07-09 15:46 EDT, Susi Lehtola
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:40:41 EST
Fedora Merge Review: pkgconfig

http://cvs.fedora.redhat.com/viewcvs/devel/pkgconfig/
Initial Owner: mclasen@redhat.com
Comment 1 Ralf Corsepius 2007-02-02 01:49:24 EST
MUSTFIX:

- Invalid debuginfos.
Please remove the strip %{bindir}/* line

- Package ships an aclocal macro
=> Requires: automake

- %makeinstall
Package supports make DESTDIR and therefore should use it.

SHOULD:
- Package wastes time on building shared libs while it only uses the static one
=> Consider %configure ... --disable-shared


Remark:
pkgconfig shipping a local copy of glib and links statically against is
gradually showing its negative sides. The amount of warnings this code emits
proves this code to be showing its age and lets it appear as really error-prone. 
Should pkgconfig support linking against an external glib, this would be preferred.
Comment 2 Matthias Clasen 2007-02-02 02:12:00 EST
No objections here.

The remark is of course moot, since pkgconfig does not support linking against
an external glib, but I'll point out that you'd have some bootstrapping fun with it,
since glibs configure script uses pkgconfig nowadays.
Comment 3 Ralf Corsepius 2007-02-02 02:29:36 EST
(In reply to comment #2)
> No objections here.
> 
> The remark is of course moot, since pkgconfig does not support linking against
> an external glib, but I'll point out that you'd have some bootstrapping fun
with it,
> since glibs configure script uses pkgconfig nowadays.
I am well aware about the bootstrapping issue.

But this is nothing which can't be overcome:
1. Build incrementally (this is what glibc, the kernel and GCC do).
2. Make glib a drop in-package to pkgconfig (This is what GCC does with some of
its components)
3. Make building and using pkgconfig's glib an option to pkg-config.

Comment 4 Ville Skyttä 2007-02-02 02:51:00 EST
(In reply to comment #1)
> - Package ships an aclocal macro
> => Requires: automake

This is strictly speaking correct, but 1) needs to be accompanied by an effort
to make sure that no non-devel packages in the distro have a dependency on
pkgconfig, or 2) the aclocal macro in this package be split to pkgconfig-devel
subpackage.  Otherwise the end result is that a bunch of nontrivial dependencies
will be brought in to regular non-devel setups where they are useless bloat.

Currently examples of non-devel packages requiring pkgconfig include metacity,
gd, gnome-icon-theme and gnome-applets, and I'm sure there's a lot more.
Comment 5 Matthias Clasen 2007-02-02 14:15:58 EST
We are not going to start adding -devel subpackages to development tool, I assume.
Comment 6 Matthias Clasen 2007-02-02 14:25:08 EST
pkgconfig-0.21-4.fc7 addresses the concerns except for the automake dependency
Comment 7 Susi Lehtola 2009-05-06 10:09:55 EDT
- Any reason why SMP make is not enabled? (make %{?_smp_mflags})

- Package includes internal glib, which is not allowed. Use BR: glib-devel and --with-installed-glib option to configure.


rpmlint output:
pkgconfig.x86_64: W: devel-file-in-non-devel-package /usr/bin/pkg-config
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

- This is kind of expected.


MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSFIX
- Use INSTALL="install -p" as argument to make install.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. N/A
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK

MUST: Permissions on files must be set properly. NEEDSFIX
- %defattr should be (-,root,root,-)

MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. OK

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSFIX
- Add ChangeLog to %doc.

MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
Comment 8 Matthias Clasen 2009-05-06 12:49:53 EDT
> - Package includes internal glib, which is not allowed. Use BR: glib-devel and
> --with-installed-glib option to configure.

Please take a minute to understand why things are the way they are...then you will see that it needs to be this way. Hint: glib is using pkg-config itself.
Comment 9 Ralf Corsepius 2009-05-06 13:47:57 EDT
(In reply to comment #8)
> > - Package includes internal glib, which is not allowed. Use BR: glib-devel and
> > --with-installed-glib option to configure.
> 
> Please take a minute to understand why things are the way they are...then you
> will see that it needs to be this way. Hint: glib is using pkg-config itself.  

Hint: Your argument is a nop.
Comment 10 Susi Lehtola 2009-05-06 15:28:02 EDT
(In reply to comment #8)
> > - Package includes internal glib, which is not allowed. Use BR: glib-devel and
> > --with-installed-glib option to configure.
> 
> Please take a minute to understand why things are the way they are...then you
> will see that it needs to be this way. Hint: glib is using pkg-config itself.  

Once there is a glib package available there's no problem to build against it. This is a cyclic dependency and is nothing new.

Just define a bootstrap variable that chooses between

a) normal case: use glib package from distro

or

b) bootstrap: build with internal glib [then build glib with the newly built pkgconfig and redo build with a)]

You only have to bootstrap once a distribution, and even that you should be able to do with a).
Comment 11 Susi Lehtola 2009-06-14 12:10:40 EDT
ping?
Comment 12 Susi Lehtola 2009-08-05 07:25:35 EDT
ping again?
Comment 13 Susi Lehtola 2010-01-01 17:46:40 EST
ping 3rd time? Please respond.
Comment 14 Matthias Clasen 2010-07-09 10:58:09 EDT
Not sure what you want from me
Comment 15 Susi Lehtola 2010-07-09 15:46:58 EDT
Created attachment 430747 [details]
Patch against rawhide spec

Suggested patch to fix the remaining issues.
Comment 16 Susi Lehtola 2010-07-09 15:54:04 EDT
Great, so the bundled glib has been removed. rpmlint output now stands at

pkgconfig.src: W: no-cleaning-of-buildroot %clean
pkgconfig.src: W: no-buildroot-tag
pkgconfig.src: W: no-%clean-section
pkgconfig.x86_64: W: incoherent-version-in-changelog 0.25-2 ['1:0.25-2.fc13', '1:0.25-2']
pkgconfig.x86_64: W: devel-file-in-non-devel-package /usr/bin/pkg-config
pkgconfig.x86_64: W: manual-page-warning /usr/share/man/man1/pkg-config.1.gz 58: warning: `I/usr/lib/pkgconfig,' not defined
pkgconfig.x86_64: W: manual-page-warning /usr/share/man/man1/pkg-config.1.gz 60: warning: `I/usr/local/share/pkgconfig' not defined
pkgconfig.x86_64: W: manual-page-warning /usr/share/man/man1/pkg-config.1.gz 154: warning: `I.pc' not defined
pkgconfig.x86_64: W: manual-page-warning /usr/share/man/man1/pkg-config.1.gz 206: warning: `I-Lx:/some/path' not defined
3 packages and 0 specfiles checked; 0 errors, 9 warnings.

Here, the only relevant warning is the incoherent version; the changelog is missing the epoch. You might want to have a look also at the man page stuff.

**

You should still add INSTALL="install -p" to make install, as some non-generated files are losing their time stamps in install.

The defattr stuff was my misunderstanding - the current syntax is also OK.

The ChangeLog reads that it isn't kept up-to-date anymore, so there's no need anymore to add it.

**

The remaining issues are cosmetic, please fix them in the next CVS push and mark this bug as closed. This package has been

APPROVED.
Comment 17 Susi Lehtola 2010-08-18 16:27:01 EDT
Well, I guess this can be closed now...

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