Bug 165964
Summary: | Review Request: intuitively. Automatic IP detection utility | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Patrice Dumas <pertusus> | ||||
Component: | Package Review | Assignee: | Dmitry Butskoy <dmitry> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | fedora-package-review | ||||
Target Milestone: | --- | ||||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
URL: | http://packages.debian.org/unstable/admin/intuitively | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2005-12-03 12:22:39 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | 165963 | ||||||
Bug Blocks: | 163779 | ||||||
Attachments: |
|
Description
Patrice Dumas
2005-08-15 12:37:06 UTC
An updated srpm with the correct url reported by Spot is here: http://www.environnement.ens.fr/docs/fc-srpms/intuitively-0.7-3.src.rpm From the web page at the new URL:: "Download the latest version from your local debian mirror, for instance ftp.uninett.no" So it appears the tarball source is a debian package. * It is unfortunate that libpcap does not "Provides: libpcap-devel = %version-%release", but libnet does, so you ought to prefer "BuildRequires: libnet-devel" over your "Buildrequires: libnet". * Better summary would be: Automatic IP detection utility Basically that's what the first broken sentence of the description contains. ;) * Build fails here: checking for flex... no checking for lex... no ./configure: line 2415: flex: command not found * I recommend marking the config file "noreplace", since the included defaults ask for customisation. * The included config file refers to a non-existant directory /usr/share/intuitively, albeit only in comments. This is confusing. This should be fixed in: http://www.environnement.ens.fr/docs/fc-srpms/intuitively-0.7-4.src.rpm I changed /usr/share to /opt in the config file, is it enough? > I changed /usr/share to /opt in the config file, is it enough?
Why don't move /opt stuff to /usr/share ? It would be more usual way of packaging...
There is nothing packaged in /opt... On the contrary... I changed /usr/share to /opt in a config file to show that it is not in the package... Remarks & nitpicks: - use '-q' for %setup - instead of the patch (patch0), just specify "sysconfdir=..." for "make install" - instead of patch1, just use "sed -i 's/.../..../' filename" at prep stage - there is a garbage in man pages, because instead of "docbook2man file.sgml" a wrong "docbook2man file.sgml > file.NUM" is used. Therefore manuals should be properly re-created in the end of %build stage. - IMO it is better to use %{name} and wildcards for %files - Is it useful to include intuitively.conf.dist into the docs? It seems to be the same as /etc/intuitively/intuitively.conf ... Created attachment 121492 [details]
Suggested changes for spec-file
Feel free to avoid some of them :)
(In reply to comment #7) The new srpm should appear shortly at http://www.environnement.ens.fr/perso/dumas/fc-srpms/intuitively-0.7-5.src.rpm > Remarks & nitpicks: > > - use '-q' for %setup done > - instead of the patch (patch0), just specify "sysconfdir=..." for "make install" it won't work because @sysconfdir@ is also used in the code. > - instead of patch1, just use "sed -i 's/.../..../' filename" at prep stage done > - there is a garbage in man pages, because instead of "docbook2man file.sgml" a > wrong "docbook2man file.sgml > file.NUM" is used. Therefore manuals should be > properly re-created in the end of %build stage. done > - IMO it is better to use %{name} and wildcards for %files I used %{name}, but only in the %files section, I think it is less readable with %{name} everywhere. I avoided wildcards (except for manpages), because I think that with the files explicitely specified, it is easier to see that something sneaked in. > - Is it useful to include intuitively.conf.dist into the docs? It seems to be > the same as /etc/intuitively/intuitively.conf ... Yep it is the same but I think it is better to keep it because the user should rewrite the /etc/intuitively/intuitively.conf. >> - instead of the patch (patch0), just specify "sysconfdir=..." for "make >> install" > it won't work because @sysconfdir@ is also used in the code. Where?... I can find it only in Makefile.in files... Anyway, each @sysconfdir@ in the code must be substituted to usual %{_sysconfdir} (without $RPM_BUILD_ROOT prefix), and configure script does it properly. Therefore "add-DEST_DIR-patch" is not needed for code itself. This patch is needed for install time only. Oh, you're right. I wrongly believed that sysconfdir was passed to configure, but it isn't the case... Thanks. It should be fixed in the srpm that should appear shortly at: http://www.environnement.ens.fr/perso/dumas/fc-srpms/intuitively-0.7-6.src.rpm Works. MUST/SHOULD items OK APPROVED. One suggestion: There are "Work" and "Home" locations in the current intuitively.conf file. "intuitively" reports "Cannot chdir to /etc/intuitively/Work/." because of missed subdirs. IMO it would be better to create /etc/intuitively/default subdir, rename one location to "default", and comment out all another locations. It will satisfy "intuitively" here and also will hint end-user to create his own subdir if needed... Well I think the the real issue is the error message "Cannot chdir to /etc/intuitively/Work/.", there should be a check that the directory exists prior doing the chdir. I don't think it is a good idea to ship with a directory in the default case because intuitively don't need this directory. I plan on doing a patch since a long time but it's not high priority for me. So I will add the package first and do the patch when I get time. |