Bug 732218 - Review Request: travelccm - C++ Travel Customer Choice Model Library
Review Request: travelccm - C++ Travel Customer Choice Model Library
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Thomas Spura
Fedora Extras Quality Assurance
:
Depends On: 702987
Blocks: 890772
  Show dependency treegraph
 
Reported: 2011-08-20 19:32 EDT by Denis Arnaud
Modified: 2015-04-06 12:02 EDT (History)
4 users (show)

See Also:
Fixed In Version: travelccm-0.5.0-2.fc16
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-10-12 20:47:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
tomspur: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Denis Arnaud 2011-08-20 19:32:21 EDT
Spec URL:
http://denisarnaud.fedorapeople.org/sim/travelccm/travelccm-0.5.0-1.spec
SRPM URL:
http://denisarnaud.fedorapeople.org/sim/travelccm/travelccm-0.5.0-1.fc15.src.rpm
Description:
TravelCCM aims at providing a clean API, and the corresponding C++ implementation, for choosing one item among a set of travel solutions, given demand-related characteristics (e.g., Willingness-To-Pay, preferred airline, preferred cabin, etc.).

The TravelCCM C++ library implements some simple Customer Choice Models (CCM), as referenced in the literature (PhD dissertations at MIT, for instance: http://dspace.mit.edu).

The TravelCCM C++ library exposes a simple, clean and object-oriented, API. For instance, the choose() method takes, as input, both a structure representing the travel request (e.g., "from Washington, DC, US, to Beijing, China, on the 25th of May") and a list of travel solutions (as provided by the Airline Schedule Manager project: http://sourceforge.net/projects/air-sched), and yields, as output, the chosen item.

The output can then be used by other systems, for instance to book the corresponding travel or to visualize it on a map and calendar and to share it with others.
Comment 1 Thomas Spura 2011-09-23 10:56:24 EDT
Review:

Good:
- name ok
- rpmlint ok (except spec naming below)
- BR ok
- group ok
- R ok
- parallel make is used
- check is there
- ldconfig called
- libs correctly installed
- no static libs
- no *.la
- %files ok
- sources match upstream (sha512): 20894cc43b0fb5902e30721c10cd3a1c38a1f6b5503e486a92ba8dfb9ffd9a01e25da5a64c56835da6cb47b22ee6d09f921200d58c335c619ca4841b1b6d86ab  travelccm-0.5.0.tar.bz2


MUST:
- spec has to be travelccm.spec without the version, but I'm sure, you'll
  change that on importing.
  See also rpmlint:
  $ rpmlint ~/rpmbuild/SRPMS/travelccm-0.5.0-1.fc15.src.rpm \
  ~/rpmbuild/RPMS/x86_64/travelccm-* \
  ~/rpmbuild/RPMS/noarch/travelccm-doc-0.5.0-1.fc15.noarch.rpm 
  travelccm.src: E: invalid-spec-name
  5 packages and 0 specfiles checked; 1 errors, 0 warnings.

SHOULD:
- can the permissions be fixed upstream? ;)
- I still think this would be better:
  %if ! (0%{?rhel} > 5)
  BuildArch:     noarch
  %endif
  So the doc package will be noarch on fedora and rhel > 5.
  Please reconsider, but only SHOULD here...
- please add proper LGPLv2+ headers

Question:
- Why are you running ctest, but suggest to run "make check" in README for checking? ;)

_____________________________________________________________________


APPROVED, when you fix spec renaming on importing
Comment 2 Denis Arnaud 2011-09-23 13:55:56 EDT
Thanks for the review!

1. Yes, of course, I will change the specification file name to travelccm.spec. As a matter of fact, locally, to test the build of the package, I usually do a symbolic link on the latest version of the specification file (e.g., travelccm.spec -> travelccm.spec-0.5.0-1.spec). That avoids confusion around the version of the specification file.

2. The permissions can be fixed upstream, of course. Indeed, they are already fixed (I have to check to be 100% sure, but normally, they are). I will remove that part from the specification file (as it has become useless).

3. For the doc sub-package, I guess that you meant:
  %if ! (0%{?rhel} < 6)
  BuildArch:     noarch
  %endif
It makes sense to me, and I will alter the specification file.

4. About the LGPLv2+ headers, you mean that I should add them into the source code?
Comment 3 Thomas Spura 2011-09-23 14:56:48 EDT
(In reply to comment #2)
> Thanks for the review!
> 
> 1. Yes, of course, I will change the specification file name to travelccm.spec.
> As a matter of fact, locally, to test the build of the package, I usually do a
> symbolic link on the latest version of the specification file (e.g.,
> travelccm.spec -> travelccm.spec-0.5.0-1.spec). That avoids confusion around
> the version of the specification file.

Yes, it will only always cause a rpmlint warning, when you link to the versioned spec. Maybe rsyncing with "-L" would help here.

> 3. For the doc sub-package, I guess that you meant:
>   %if ! (0%{?rhel} < 6)
>   BuildArch:     noarch
>   %endif
> It makes sense to me, and I will alter the specification file.

Yes, of course :(

> 
> 4. About the LGPLv2+ headers, you mean that I should add them into the source
> code?

Hmm, I ought to remember, that's a SHOULD to ask upstream for doing so, e.g.:
http://ball-trac.bioinf.uni-sb.de/ticket/220
Comment 4 Denis Arnaud 2011-09-24 13:29:18 EDT
Feedback has been taken into account:

===================================================
Spec URL:
http://denisarnaud.fedorapeople.org/sim/travelccm/travelccm-0.5.0-2.spec
SRPM URL:
http://denisarnaud.fedorapeople.org/sim/travelccm/travelccm-0.5.0-2.fc15.src.rpm
===================================================

Note that:
--------------
 %if ! (0%{?rhel} < 6)
 BuildArch:     noarch
 %endif

does not work on Fedora because %{rhel} is not defined (and therefore the whole condition holds false).
Hence, I have used:
 %if 0%{?fedora} || 0%{?rhel} > 5
 BuildArch:      noarch
 %endif
--------------

Linking travelccm.spec to travelccm-0.5.0-2.spec, in order to build (with rpmbuild -bs/-ba) and to test (with rpmlint) RPM packaging, fully works: rpmlint does not report any warning.

Indeed, it allows me tracking all the different versions for all the review requests and bugs, in a very convenient way: https://github.com/denisarnaud/fedorareviews/tree/trunk/sim/travelccm/travelccm_732218
It could give ideas to some other package maintainers :)
Comment 5 Thomas Spura 2011-10-01 07:27:25 EDT
(In reply to comment #4)
> Feedback has been taken into account:

Looks good now.

> Hence, I have used:
>  %if 0%{?fedora} || 0%{?rhel} > 5
>  BuildArch:      noarch
>  %endif

Yes, fine now.

> Indeed, it allows me tracking all the different versions for all the review
> requests and bugs, in a very convenient way:
> https://github.com/denisarnaud/fedorareviews/tree/trunk/sim/travelccm/travelccm_732218
> It could give ideas to some other package maintainers :)

Hmm, when you use git, I'd only modify travelccm.spec and let git do its job with tracking the different versions :)

In your case you should upload the travelccm.spec link to fedorapeople, so a reviewer will get that first (and see the history when manually going there and looking at the versioned specs).

Feel free to request a new package.
Comment 6 Denis Arnaud 2011-10-01 14:30:14 EDT
New Package SCM Request
=======================
Package Name: travelccm
Short Description: C++ Travel Customer Choice Model (CCM) Library
Owners: denisarnaud
Branches: f14 f15 f16 el4 el5 el6
InitialCC: denisarnaud
Comment 7 Jon Ciesla 2011-10-03 08:22:05 EDT
Git done (by process-git-requests).
Comment 8 Fedora Update System 2011-10-04 09:15:41 EDT
travelccm-0.5.0-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/travelccm-0.5.0-2.fc16
Comment 9 Fedora Update System 2011-10-04 09:16:29 EDT
travelccm-0.5.0-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/travelccm-0.5.0-2.fc15
Comment 10 Fedora Update System 2011-10-04 09:49:50 EDT
travelccm-0.5.0-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/travelccm-0.5.0-2.fc14
Comment 11 Fedora Update System 2011-10-04 16:47:40 EDT
travelccm-0.5.0-2.fc16 has been pushed to the Fedora 16 testing repository.
Comment 12 Fedora Update System 2011-10-12 20:47:02 EDT
travelccm-0.5.0-2.fc15 has been pushed to the Fedora 15 stable repository.
Comment 13 Fedora Update System 2011-10-12 20:50:52 EDT
travelccm-0.5.0-2.fc14 has been pushed to the Fedora 14 stable repository.
Comment 14 Fedora Update System 2011-10-13 00:38:45 EDT
travelccm-0.5.0-2.fc16 has been pushed to the Fedora 16 stable repository.

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