Bug 239924 (xournal) - Review Request: xournal - Take notes, sketch and annotate PDFs
Summary: Review Request: xournal - Take notes, sketch and annotate PDFs
Keywords:
Status: CLOSED NEXTRELEASE
Alias: xournal
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-05-12 14:26 UTC by Rick L Vinyard Jr
Modified: 2010-05-28 18:41 UTC (History)
0 users

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2007-06-01 20:43:04 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
mock build log of xournal 0.3.3-3 on F-7 i386 (59.56 KB, text/plain)
2007-05-30 14:21 UTC, Mamoru TASAKA
no flags Details

Description Rick L Vinyard Jr 2007-05-12 14:26:53 UTC
Spec URL: http://miskatonic.cs.nmsu.edu/pub/xournal.spec
SRPM URL: http://miskatonic.cs.nmsu.edu/pub/xournal-0.3.3-1.fc7.src.rpm

Description:
Xournal is an application for notetaking, sketching, keeping a journal and 
annotating PDFs. Xournal aims to provide superior graphical quality (subpixel 
resolution) and overall functionality.

Comment 1 Rick L Vinyard Jr 2007-05-19 04:29:21 UTC
Spec URL: http://miskatonic.cs.nmsu.edu/pub/xournal.spec
SRPM URL: http://miskatonic.cs.nmsu.edu/pub/xournal-0.3.3-2.fc7.src.rpm

Added gnome and kde mime support.

Comment 2 Mamoru TASAKA 2007-05-21 08:35:27 UTC
* MUST/Should fix
? Question
! Suggestion/Note/etc

For 0.3.3-2:
* macros
  - Please choose to either use or not use macros for 
    commands.
    For example, you use %{__install} instead of "install",
    while you use "make" "rm" "mkdir -p" "ln -s" "cat"
    directly.

* Timestamps
  - Keep timestamps on files which are not created or
    modified during rebuild.
    * Please use %{__install} with "-p" option.
    * For this package, the following usage can save
      timestamps of many files.
----------------------------------------------------------
%{__make} install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p"
----------------------------------------------------------

* Desktop file
  - Both category "Application" "X-Fedora" are now deprecated
    and should be removed.

! Create text files by %{__cat} vs add as Source
  - Would you consider to add text files as Source, not to
    create them by cat command at install stage?
    Adding text files as Source prevents timestamps of 
    those files from being changed for each rebuild.

! Possible mistake in the future
  - The build log says:
------------------------------------------------------------------
gcc <snip> -DPACKAGE_LOCALE_DIR=\""/usr//locale"\" <snip>
------------------------------------------------------------------
    Currently this package does not have any gettext .mo
    files so this is not a problem, however when any .mo files
    are added in the future, please fix this so that
    "PACKAGE_LOCALE_DIR" correctly points to %{_datadir}/locale.

* Substantially duplicate files
  - The files under (doc)/manual.html, (doc)/pixmaps/ and
    those under %{_datadir}/%{name}/html-doc/, 
    %{_datadir}/%{name}/pixmaps/ are the same and either of
    them (perhaps formar) should be removed.

* Unnecessary scriptlets
  - The following is not needed because the commands in the
    scriptlets are called with || :
-----------------------------------------------------------------
Requires(post):      desktop-file-utils >= 0.8
Requires(postun):     desktop-file-utils >= 0.8
-----------------------------------------------------------------

* Source URL
  - Specify the URL of Source1/2, otherwise write as a comment
    how you created Source1/2.

* configure call
  - During the build, configure is called twice. Please call
    one time
    (According to autogen.sh, this may be solved by setting
     NOCONFIGURE).

Comment 3 Mamoru TASAKA 2007-05-25 17:39:19 UTC
ping?

Comment 4 Rick L Vinyard Jr 2007-05-29 20:31:48 UTC
Thanks for the feedback. This should have everything fixed.

Spec URL: http://miskatonic.cs.nmsu.edu/pub/xournal.spec
SRPM URL: http://miskatonic.cs.nmsu.edu/pub/xournal-0.3.3-3.fc7.src.rpm

Changelog:
- Changed all commands to macros
- Removed icon sources and create icons in spec from xournal icon
- Added 64x64 and 128x128 icons
- Consolidated icon directories with wildcards
- Added timestamp preservation to install
- Removed desktop categories Application and X-Fedora
- Added NOCONFIGURE to autogen.sh to stop auto-conf from running twice
- Removed desktop-file-utils post and postun requires
- Removed manual from doc section; it is already installed by the package
- Changed xournal.desktop, xournal.xml and x-xoj.desktop from here documents to
files
- Add ImageMagick buildrequires for convert command
- Separated BuildRequires into one per line for easier reading
- Added PACKAGE_LOCALE_DIR CFLAG to configure


Comment 5 Mamoru TASAKA 2007-05-30 14:21:12 UTC
Created attachment 155686 [details]
mock build log of xournal 0.3.3-3 on F-7 i386

This time you can see as in attached build log:

* -DPACKAGE_DATA_DIR is not fixed.
* Fedora specific compilation flags are not honored.

Comment 6 Rick L Vinyard Jr 2007-05-30 21:16:04 UTC
I forgot about optflags when I messed with CFLAGS. This one has it fixed, along
with a specific override for PACKAGE_DATA_DIR as well that will always map to a
proper Fedora directory.

Spec URL: http://miskatonic.cs.nmsu.edu/pub/xournal.spec
SRPM URL: http://miskatonic.cs.nmsu.edu/pub/xournal-0.3.3-4.fc7.src.rpm

Changelog:
- Added optflags and PACKAGE_DATA_DIR to CFLAGS


Comment 7 Mamoru TASAKA 2007-05-31 15:35:07 UTC
Okay!

-------------------------------------------------
  This package (xournal) is APPROVED by me.
-------------------------------------------------

Comment 8 Rick L Vinyard Jr 2007-05-31 18:10:16 UTC
Thanks for the review and all the help.

New Package CVS Request
=======================
Package Name: xournal
Short Description: Take notes, sketch and annotate PDFs
Owners: rvinyard.edu
Branches: FC-6 F-7
InitialCC:

Comment 9 Jason Tibbitts 2007-06-01 18:56:47 UTC
CVS done.

Comment 10 Rick L Vinyard Jr 2007-06-01 20:43:04 UTC
Thanks again for the review and the help... another app is on it's way.

Comment 11 Rick L Vinyard Jr 2010-05-28 15:52:39 UTC
Package Change Request
======================
Package Name: xournal
New Branches: el5
Owners: rvinyard

Comment 12 Jason Tibbitts 2010-05-28 16:07:07 UTC
Double checking: you want an EL-5 branch but not an EL-6 branch?

Comment 13 Rick L Vinyard Jr 2010-05-28 18:25:00 UTC
(In reply to comment #12)
> Double checking: you want an EL-5 branch but not an EL-6 branch?    

I didn't realize it was time for EL-6 as well.

Yes to EL-6 if they're already being made.

Comment 14 Jason Tibbitts 2010-05-28 18:41:31 UTC
No problem.  CVS done.


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