Bug 167147 - Review Request: aqsis - 3D Rendering system
Review Request: aqsis - 3D Rendering system
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
http://www.aqsis.org
: Reopened
: 202946 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-30 19:15 EDT by Tobias Sauerwein
Modified: 2009-07-09 23:38 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-04 19:07:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)
New spec for the upcomming aqsis release. (4.02 KB, application/octet-stream)
2006-12-03 12:34 EST, Tobias Sauerwein
no flags Details

  None (edit)
Description Tobias Sauerwein 2005-08-30 19:15:02 EDT
Spec Name or Url: aqsis.spec
SRPM Name or Url: aqsis-1.1.0-20050816cvs.src.rpm
Description: Aqsis is a high quality, photorealistic, 3D rendering solution. It complies with the 
Renderman® interface standard defined by Pixar.
Comment 1 Ignacio Vazquez-Abrams 2005-08-30 19:47:37 EDT
Please give actual URLs to the SRPM and spec file, not just filenames.
Comment 3 Tobias Sauerwein 2005-09-07 09:13:47 EDT
Description: Aqsis is a high quality, RenderMan-compliant, 3D rendering solution.
Comment 4 Ralf Corsepius 2005-09-08 02:06:45 EDT
NEEDSWORK

I regret having to tell you that your spec is not ready for inclusion to FE and
that I am having doubts if this package is ready for inclusion.

Blockers:
* devel files not split out into a devel package.
* No URL in Source:
* Missing BuildRequire:'s.
* Bogus Require:'s.
* Packager:, Vendor:-tag
...

Finally: You seem to be trying to package a _local_ CVS-snapshot of what even
the aqsis homepage (http://www.aqsis.org -> downloads) describes as:
"Unstable source drop of the current bleeding edge development. Not suitable for
production use, this version contains many "work in progress" changes."

IMO:
1. You should not base packages on local tarballs. Packages should be based on
public tarballs. If you feel you can't avoid changes, supply patches against
public tarballs.
2. FE packages should not be based on unstable snapshots, unless there is a
compelling reason to do so (E.g. API is frozen, a release is immediately ahead).

From what I see on the aqsis pages, they don't give any timeline for a new
release, instead, they explicitly warn about 1.1.0-snapshots, declare its API
unstable and recommend to use the 1.0.0 release.
Comment 5 Jason Tibbitts 2006-02-18 18:33:27 EST
I can't even download the proposed spec or SRPM to take a look.  Should this
review request be considered withdrawn?  (Is there even a procedure for
withdrawing a review request?)
Comment 6 Ralf Corsepius 2006-02-23 01:34:37 EST
No feedback from submitter for more than 5 months.
Comment 7 Tobias Sauerwein 2006-02-23 04:05:29 EST
I am really sorry about this. I had my practical semester and couldn't take my linux box with me. But I'll 
soon have access to it again so I can continue my work on the spec/SRPM/RPM.
Comment 8 Tobias Sauerwein 2006-12-03 12:34:21 EST
Created attachment 142685 [details]
New spec for the upcomming aqsis release.

This is the new spec I promised a long time ago. I am very sorry about that it
took so long, but we moved over to scons and until recently it wasn't possible
to build proper RPM's for our scons system.
Comment 9 Denis Leroy 2006-12-03 17:39:42 EST
*** Bug 202946 has been marked as a duplicate of this bug. ***
Comment 10 Ralf Corsepius 2006-12-03 23:54:29 EST
(In reply to comment #8)
> Created an attachment (id=142685) [edit]
> New spec for the upcomming aqsis release.

Please provide full urls to spec and src.rpm. 

It's very inconvenient to reviewers having to dig for the individual pieces a
package consists of. In this case it renders reviewing this package impossible,
because you spec doesn't point to a valid source tar ball.
Comment 11 Gianluca Sforna 2006-12-04 02:55:45 EST
Moreover, if this is your first submission, it should also block bug 177841
(FE-NEEDSPONSOR)
Comment 12 Tobias Sauerwein 2006-12-04 03:26:14 EST
Ok, sorry about that. Here is the spec and src.rpm.

http://www.cgtobi.de/aqsis/aqsis.spec
http://www.cgtobi.de/aqsis/aqsis-1.1.0-1.src.rpm

Gianluca Sforna: Thanks for pointing out the FE-NEEDSPONSOR thing.
Comment 13 Ralf Corsepius 2006-12-04 04:31:20 EST
Just some initial comments:

MUSTFIX:
- No URL for Source0
 => Sources can not be verified.

- Building doesn't acknowledge RPM_OPT_FLAGS
 => Package will be miscompiled.

- Drop shipping static libs.

- Missing BuildRequires: scons
 => Package doesn't build in mock

- Scons produces broken compilation rules
  ... -I/usr/include -I/usr/include -I/usr/include ...
  -I/usr/include is in gcc's system include path and may desturb GCC.

- Most of the "Requires:" probably are not needed.

- *.pyc and *pyo in /usr/bin

Upstream issues (non-blocking):
- mal designed shared library: /usr/lib/libaqsis.so
  - Missing SONAME
  - Improperly versioned file name. Maintaining this package will become a
    nightmare in longer terms.

- A pretty impressive amount of "punned pointer warnings".
  => The package is likely not to work with newer GCCs.

- Using obsolete c++-headers

- build/rib/api/ri_cache.inl:4106:2: warning: no newline at end of file
Comment 14 Tobias Sauerwein 2006-12-04 05:05:21 EST
About the "No URL for Source0": The URL will be provided as soon as aqsis 1.1.0 is released. (we're allmost 
there)

Can you elaborate the "Drop shipping static libs" comment, please?
Comment 15 Ralf Corsepius 2006-12-04 05:35:04 EST
(In reply to comment #14)
> About the "No URL for Source0": The URL will be provided as soon as aqsis
1.1.0 > is released. (we're allmost there)
I am not going to review a package based on non-released sources.

Either upstream will have to release their source first, or you will have to
implement a set of patches, based on older released version.

> Can you elaborate the "Drop shipping static libs" comment, please?
http://fedoraproject.org/wiki/PackagingDrafts/StaticLinkage
Comment 16 Paul Gregory 2006-12-04 05:56:25 EST
> I am not going to review a package based on non-released sources.
> 
> Either upstream will have to release their source first, or you will have to
> implement a set of patches, based on older released version.

This is a big of a quandary, the build and packaging is part of our source, so
either we...

1. Release the source with the RPM packaging not working, then modify it to pass
RPM compliance requirements, resulting in the released source and the RPM valid
source being out of sync. Not nice, especially as to fit all the RPM
requirements it seems will result in changes to the build system, i.e.
significant changes from the released source.

or

2. Don't release the source until the RPM compliance is met, resulting in the
RPM version being in release step with the non-RPM version, which is much more
preferable from a management point of view. But the reviewer has stated that
they are not prepared to review RPM compliance until the source is released.

Chicken and egg. It seems that this policy makes it very difficult to release in
a clean and manageable way if your release platforms include FC RPM.

Any suggestions welcome.
Comment 17 Dominik 'Rathann' Mierzejewski 2006-12-04 06:33:39 EST
I believe option 2 should be followed in this case, because, as you said, 1.1.0
is about to be released. Still, you should provide a prerelease snapshot source
on your site, even if only for the purpose of this package.
Comment 18 Denis Leroy 2006-12-04 11:06:15 EST
Same here, I would recommend to submit an SRPM based on a CVS snapshot, if only
for the goal of going through the review process. After the package is approved,
you can update it with the official 1.1.0 release. The reason is we can't review
a package whose source cannot be verified upstream.

I would recommend using a version such as "1.1.0-0.cvsYYYYMMDD.1.fc6". The
leading '0.' in front of the snapshot date ensures that the version will always
be inferior to "1.1.0-1" used when 1.1.0 is officially released.

I'm having some problems making this aqsis package with k3d on my system: aqsis
doesn't find the correct shaders no matter which one i try, which results in a
white object rendered (ERROR: Shader "k3d_plank" not found), even though this
works with my aqsis 1.0.1 package. I'm not sure if this is a k3d problem or not.
Comment 19 Paul Gregory 2006-12-04 11:23:41 EST
> The reason is we can't review
> a package whose source cannot be verified upstream.

What exactly do you mean by 'verified upstream'???
Comment 20 Jason Tibbitts 2006-12-04 11:27:54 EST
Just a note that the version/release strings to use for snapshots are mandated
by the Naming Guidelines and differ from the example above:

http://fedoraproject.org/wiki/Packaging/NamingGuidelines#SnapshotPackages
Comment 21 Ralf Corsepius 2006-12-04 11:31:23 EST
(In reply to comment #19)
> > The reason is we can't review
> > a package whose source cannot be verified upstream.
> 
> What exactly do you mean by 'verified upstream'???

We need to use a globally available source for a source tarball, which we can
compare the tarball inside of Fedora's buildsystem against. Otherwise arbrary
"jerks" could tar up some files and put them into Fedora, claiming they were yours.
Comment 22 Paul Gregory 2006-12-04 11:36:17 EST
I see, that makes sense, sort of. I'll have to take a look at our build current
system and see what we can do. Maybe if we sort out the outstanding issues
mentioned above, then freeze for review.
Comment 23 Tobias Sauerwein 2006-12-15 10:18:17 EST
We just released aqsis-1.2.0alpha1. We fixed all the issued mentioned above. You can download the 
latest aqsis.spec right from the SVN server.
http://aqsis.svn.sourceforge.net/viewvc/*checkout*/aqsis/tags/release-1.2.0alpha1/aqsis/
distribution/linux/rpm/aqsis.spec

And here is the link to the official aqsis-1.2.0alpha1.tar.gz:
http://download.aqsis.org/stable/source/tar/aqsis-1.2.0alpha1.tar.gz

RPM's can be found right here:
http://cgtobi.de/aqsis/aqsis-1.2.0-0.1.alpha1.i386.rpm
http://cgtobi.de/aqsis/aqsis-1.2.0-0.1.alpha1.src.rpm
http://cgtobi.de/aqsis/aqsis-devel-1.2.0-0.1.alpha1.i386.rpm
http://cgtobi.de/aqsis/aqsis-data-1.2.0-0.1.alpha1.i386.rpm

A few comments on the aqsis.spec. This spec is for cross-distribution and works on Fedora, Suse and 
Mandrive. To make it working on Fedora FC4 we used a little workaround, using touch, to exclude 
the .pyo/.pyc files, as suggested on #fedora-extras.
Comment 24 Leon Tony Atkinson 2006-12-18 07:16:20 EST
Further to Tobi's previous comments this is a cross-distribution SPEC, targeted
at Fedora (FC5 tested), Mandriva (2006 tested) and SUSE (10.2 tested)... and
uses a couple of conditional statements to reflect this.

The 'touch' work-around (FC5 tested) was for an issue in 'rpmbuild' which seemed
to be creating unnecessary binaries for a Python script we distribute.

Our goal is to release 1.2.0 before Xmas (1 week) and we are still looking for a
sponsor should anyone be able to help - The work involved should be
little/minimal for this person.
Comment 25 Tobias Sauerwein 2006-12-21 16:23:11 EST
After discussing the spec on #fedora-extras I created a fedora-only spec. 

You can get it right here: 
http://www.cgtobi.de/aqsis/aqsis.fedora.spec
Comment 26 Mamoru TASAKA 2006-12-23 04:41:59 EST
Well, would you provide both spec/srpm of this package
for convenience? Also, each you modify spec/srpm, you must
change (increment) release number because only changing
srpm/spec without release number confuses people who
check it.

Only from I viewed the spec file in comment #25 (I have not
tried to rebuild this, please provide srpm),
* Don't write redundant Requires which are required automatically
  by libraries' dependencies.
* Don't write redundant BuildRequires which are included in
  minimal. Check:
  http://fedoraproject.org/wiki/Extras/FullExceptionList

* Please explain why -devel package should include static 
  archives
  Check "Exclusion of Static Libraries" of
  http://fedoraproject.org/wiki/Packaging/Guidelines
--------------------------------------------
%{_libdir}/%{name}/*.a
--------------------------------------------

* 
--------------------------------------------
%{_bindir}/mpanalyse.py
%exclude %{_bindir}/mpanalyse.pyo
%exclude %{_bindir}/mpanalyse.pyc
--------------------------------------------
  Would you consider to rename this python script to mpanalyse?
  Generally, putting a script named *.py is regarded to be avioded.
  Unnecessary Byte compilation of
  http://fedoraproject.org/wiki/Packaging/Python

* Requires:	%{name} = %{version}
  Why not require release dependency? 
-----------------------------------------------------------
- MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release} -----------------------------------------------------------
  from http://fedoraproject.org/wiki/Packaging/ReviewGuidelines

* %config %{_sysconfdir}/aqsisrc
  Please explain why this is not marked as %config(noreplace).
Comment 27 Tobias Sauerwein 2006-12-23 08:00:35 EST
I fixed the release number chaos and uploaded the new spec ans srpm.
http://www.cgtobi.de/aqsis/aqsis.fedora.spec
http://www.cgtobi.de/aqsis/aqsis-1.2.0-0.3.alpha1.src.rpm

I also cleaned up the BuildRequires and Requires and fixed the dependency issue.

>>  Please explain why -devel package should include static archives
We want to deliver two useful developer libraries, libri2rib, and libslxargs. Both rely on libaqsistypes, if 
we deliver them both with libaqsistypes embedded, there will be clashes if anyone wants to use both of 
them, which means we'll be delivering a whole load more libraries, and to compile a simple program 
that relies on libri2rib would mean linking with libri2rib, and libaqsistypes, which is messy.

>>  Would you consider to rename this python script to mpanalyse?
I could do it, but I'd prefere to leave it a .py because it would be in line with other platforms we support 
and it showes the user that it is a python script.

>>  Please explain why this is not marked as %config(noreplace).
We discussed that and came to the conclusion that it's more important to provide a working version 
rather keeping the users modifications. This will definitly change in future releases.
Comment 28 Mamoru TASAKA 2006-12-23 12:40:57 EST
(In reply to comment #27)
> >>  Please explain why -devel package should include static archives
> We want to deliver two useful developer libraries, libri2rib, and libslxargs.
Well, I rebuilt this, however I cannot understand what you want
to do...
Build log says that all files used in these archives are included
in libaqsis.so.1, so including libaqsis.so in -devel package 
should be enough.
And.... including _static_ archive into _shared_ dynamic library is
wrong because rpmlint complains:
----------------------------------------------------
E: aqsis shlib-with-non-pic-code /usr/lib/libaqsis.so.1.2

shlib-with-non-pic-code :
The listed shared libraries contain object code that was compiled
without -fPIC. All object code in shared libraries should be
recompiled separately from the static libraries with the -fPIC option.
----------------------------------------------------
.. You should
* compile all files used in libaqsis.so.1.2 with -fPIC. libaqsis.so.1.2
  uses compiled objects in static archives without -fPIC used, which
  is wrong.
* After it is modified so as all files used in libaqsis.so.1.2
  are compiled with -fPIC, providing static archives should not be
  necessary as all files are included in libaqsis.so.1.2.

> Both rely on libaqsistypes, if 
> we deliver them both with libaqsistypes embedded, there will 
> be clashes if anyone wants to use both of 
> them, 
  This should be fixed when libaqsis compilation is fixed.  

> >>  Would you consider to rename this python script to mpanalyse?
> I could do it, but I'd prefere to leave it a .py because 
> it would be in line with other platforms we support 
> and it showes the user that it is a python script.
  IMO that "it showes the user that it is a python script" is not
  a enough reason. Many (in my environment, almost all) python
  scripts in /usr/bin has a name without .py suffix.
  Generally,
  * Executables used in users directly (which are under normal path)
    should not have suffix as possible.
  * If a executable is normally considered to be used from other
    executables and not to be used directly by user, the executable
    may have a suffix like .py or .sh, however, generally it should
    not under normal path and should be under %{_libexecdir}.
    (For second one, check "Filesystem Layout" of
     http://fedoraproject.org/wiki/Packaging/Guidelines).

   IMO, if no special reason exists, the filename should be renamed
   also on other platforms (however, I don't care about them).

==================================================================
And.. aqsis related packages make lots of rpmlint complaint.

------------------------------------------------------------------
1. E: aqsis shlib-with-non-pic-code /usr/lib/libaqsis.so.1.2
2. W: aqsis conffile-without-noreplace-flag /etc/aqsisrc
3. E: aqsis invalid-spec-name aqsis.fedora.spec
4. W: aqsis rpm-buildroot-usage %build scons %{?_smp_mflags}
destdir=$RPM_BUILD_ROOT \
5. W: aqsis mixed-use-of-spaces-and-tabs (spaces: line 89, tab: line 6)
6. E: aqsis-data summary-too-long Example content for the open source
RenderMan-compliant Aqsis 3D rendering solution
7. W: aqsis-data no-documentation
8. E: aqsis-data non-executable-script
/usr/share/aqsis/content/ribs/features/layeredshaders/render.sh 0644
9. E: aqsis-data non-executable-script
/usr/share/aqsis/content/ribs/scenes/vase/render.sh 0644
10. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/shadercompiler/slparse/scanner.ll
11. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/texturing/plugins/png2tif/png2tif.c
12. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/displays/d_sdcBMP/d_sdcBMP.cpp
13. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/thirdparty/dbo_plane/implicit.h
14. E: aqsis-debuginfo wrong-script-end-of-line-encoding
/usr/src/debug/aqsis-1.2.0/build/thirdparty/dbo_plane/implicit.h
15. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/thirdparty/dbo_plane/dbo_plane.c
16. E: aqsis-debuginfo wrong-script-end-of-line-encoding
/usr/src/debug/aqsis-1.2.0/build/thirdparty/dbo_plane/dbo_plane.c
17. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/aqsistypes/pool.h
18. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/displays/d_exr/dspyhlpr.c
19. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/aqsistypes/noise1234.cpp
20. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/aqsistypes/noise1234.h
21. E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/rib/rib2/scanner.ll
22. E: aqsis-devel summary-too-long Development files for the open source
RenderMan-compliant Aqsis 3D rendering solution
23. W: aqsis-devel no-documentation
-----------------------------------------------------------------------
Well,
-----------------------------------------------------------------------
1. As said above, /usr/lib/libaqsis.so.1.2 uses object files without 
   -fPIC used on compilation.
2. For now, I leave at it is.
3. The name of spec file must be aqsis.spec
4. Usually using $RPM_BUILD_ROOT in build stage is wrong because this 
   frequently causes wrong results in "real" install. Consider if
   this description (i.e. using $RPM_BUILD_ROOT) can be moved at
   %install stage.
5. For indentation, you should use tabs or spaces, not both, for
   cosmetic issue.
6,22 The "Summary:" must not exceed 79 characters.
7,23 This can be ignored.
8,9 Shell scripts with shebang should have executable permissons.
    If the shell scripts should not have executable permissions,
    then shebangs should be removed.
10-21: These usually means:
    A. source code should not have executable permissions.
    B. A file with Windows-like end-of-line encodings should be fixed
       so as to have Unix end-of-line encoding.

I have not checked these packages by installing them because I think
there is a lot of things to be fixed before doing so....
Comment 29 Mamoru TASAKA 2006-12-23 12:45:33 EST
(In reply to comment #28)
> (In reply to comment #27)
> .. You should
> * compile all files used in libaqsis.so.1.2 with -fPIC. libaqsis.so.1.2
>   uses compiled objects in static archives without -fPIC used, which
>   is wrong.

Or, you should change all static archives to shared dynamic libraries
with -fPIC used correctly, and have libaqsis.so linked against them.
Comment 31 Mamoru TASAKA 2006-12-29 00:56:10 EST
(In reply to comment #30)

> http://kwizart.free.fr/fedora/patches/aqsis-1.2.0-long_86_64.patch
> http://kwizart.free.fr/fedora/SPECS/aqsis.spec
>
http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis-1.2.0-0.2.alpha1.kwizart.fc6.src.rpm

Is this regarded as the revised version of comment #27?
Is so, please "increase" release.
And.. none of the issues I pointed out are fixed.
Comment 32 Michael Schwendt 2007-01-09 17:42:41 EST
Further:

> %{_libdir}/%{name}/*.so

The directory %{_libdir}/%{name} itself is not included.

> %install
> rm -rf $RPM_BUILD_ROOT
> export CFLAGS=$RPM_OPT_FLAGS
> export CXXFLAGS=$RPM_OPT_FLAGS

If during %install CFLAGS or CXXFLAGS are used to compile something,
that would be wrong. The two exports can be deleted.
Comment 34 Nicolas Chauvet (kwizart) 2007-01-16 20:22:21 EST
It will not build on x86_64!
You should add:
ExclusiveArch:  i386
Or
Only patch it on x86_64 with an experimental support
%ifarch x86_64
%patch0 -p1
%endif

I also wonder why the plugin dir is in /usr/lib in x86_64 if your spec file
libdir=%{_libdir} it will work fine depending on the choice above...


Some build quotes:
--------------------------
...
tall_prefix=/usr sysconfdir=/etc no_rpath=true build
scons: Reading SConscript files ...
Looking for build directory for platform 'linux2'
Exact match not found, finding closest guess
No match found, looking for 'default' directory
Found configuration directory platform/default, will use that
...
build/shadercompiler/shadervm/shadervm.cpp: In member function 'void
Aqsis::CqShaderVM::LoadProgram(std::istream*)':
build/shadercompiler/shadervm/shadervm.cpp:1108: error: cast from 'void*' to
'int' loses precision
scons: *** [build/shadercompiler/shadervm/shadervm.os] Error 1
scons: building terminated because of errors.

Comment 35 Nicolas Chauvet (kwizart) 2007-01-16 23:29:53 EST
About debuginfo rpmlint errors,
you can take a look on this spec:

SPEC
http://kwizart.free.fr/fedora/SPECS/aqsis.spec
Comment 36 Mamoru TASAKA 2007-01-17 08:53:50 EST
(In reply to comment #35)
> SPEC
> http://kwizart.free.fr/fedora/SPECS/aqsis.spec

I will check this spec file.
Comment 37 Mamoru TASAKA 2007-01-17 11:17:36 EST
Well, much improvement for this package!!

A. Packaging issue (mainly in
   http://fedoraproject.org/wiki/Packaging/Guidelines )

So, first full review.
(Again note: please increase the release number 
 when you change the spec file, and reset the release
 number when you use new source).

* Requires:
-------------------------------------------------------
Requires: fltk >= 1.1.0, libjpeg >= 6b, libtiff >= 3.7.1, OpenEXR
-------------------------------------------------------
  These should not be needed. Libraries' dependencies automatically
  checked by rpmbuild correctly pull these dependencies.

* Documentation
  - "INSTALL" should not be necessary. This is needed for
    people who want to rebuild this package by themselves.

* ChangeLog
  - 1.2.0-0.6alpha2
    should be 1.2.0-0.6.alpha2

* Compilation flags
  - Well, please look at the build log.
    This spec file:
    1. Once compile at %build stage using scons. At this time
       compiler uses Fedora specific compilation flags.
    2. Next at %install stage, scons tries to compile all targets
       once more!! At this stage, compiler does not use Fedora
       specific compilation flags, so this is wrong.

    ? Doesn't scons accept the argument like --skip-build at
      install stage like usual "setup.py" written in python?
    ? Or are there any way to avoid twice compilation?
    - If not, move all compilation using scons to %install stage,
      compiling twice is redundant.

* Encodings:
  - Please change the encoding of the following file(s)
    to UTF-8.
-----------------------------------------------------
    /usr/share/doc/aqsis-1.2.0/AUTHORS:         ISO-8859 English text
-----------------------------------------------------

  = (not a blocker) If you are upstream, please change all
     text files to UTF-8 for next tarball.

= License (okay)
  = Some files are licensed not under LGPL or GPL.
    ./displays/d_sdcBMP/d_sdcBMP.cpp
    ./displays/d_sdcWin32/d_sdcWin32.cpp
    ./shadercompiler/slpp/pp*
    ./displays/d_exr/dspyhlpr.c
    ./displays/d_sdcWin32/interface.c
    ./displays/d_sdcWin32/d_sdcWin32.h
    = None of them conflicts with GPL.
Comment 38 Mamoru TASAKA 2007-01-17 11:39:31 EST
By the way
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work.
But before I accept this package, someone (I am a candidate)
must sponsor you.

Once you are sponsored, you have the right to formally review other
submitters' review request and approve the packages.
For this reason, the person who want to be sponsored (like you)
are required to "show that you have an understanding
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/Extras/HowToGetSponsored


Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" (at the time you are not sponsored, you cannot do
   a formal review) of other person's review request.

When you submitted a new review request or have pre-reviewed other person's
review request, please write the bug number on this bug report so that I
can check your comments or review request.

Fedora Extras package review requests which are waiting for someone to review
can be checked on:
https://bugzilla.redhat.com/bugzilla/showdependencytree.cgi?id=FE-NEW&hide_resolved=1

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 39 Michael Schwendt 2007-01-17 12:05:28 EST
> It will not build on x86_64!
> You should add:
> ExclusiveArch:  i386

Usually, this is wrong. If you know it doesn't build on x86_64,
you do

 ExcludeArch: x86_64

and add a comment which contains the link to a ticket in
bugzilla.redhat.com where the build problem is explained.
If, on the contrary, you use ExclusiveArch, you lock out
all other architectures, too.
Comment 40 Paul Gregory 2007-01-17 18:06:03 EST
(In reply to comment #37)
> * Compilation flags
>   - Well, please look at the build log.
>     This spec file:
>     1. Once compile at %build stage using scons. At this time
>        compiler uses Fedora specific compilation flags.
>     2. Next at %install stage, scons tries to compile all targets
>        once more!! At this stage, compiler does not use Fedora
>        specific compilation flags, so this is wrong.
> 
>     ? Doesn't scons accept the argument like --skip-build at
>       install stage like usual "setup.py" written in python?
>     ? Or are there any way to avoid twice compilation?
>     - If not, move all compilation using scons to %install stage,
>       compiling twice is redundant.
This is a regression of a problem we had before. The problem is reintroduced by
the 'fix' to a comment a couple of posts above, about removing the specification
of CFLAGS and CXXFLAGS for the 'install' pass. By removing those, the command
line to the compiler changes, which SCons sees as a dependency change, so
rebuilds all files with the new command line. The only way round this is to
leave the CFLAGS and CXXFLAGS declarations in the 'install' pass so that the
settings seen by SCons are identical to those used during the 'build' pass.

Paul Gregory
Comment 41 Mamoru TASAKA 2007-01-17 22:13:51 EST
(In reply to comment #40)
> (In reply to comment #37)
> > * Compilation flags
> >   - Well, please look at the build log.
> >     This spec file:
> >     1. Once compile at %build stage using scons. At this time
> >        compiler uses Fedora specific compilation flags.
> >     2. Next at %install stage, scons tries to compile all targets
> >        once more!! At this stage, compiler does not use Fedora
> >        specific compilation flags, so this is wrong.
> > 
> This is a regression of a problem we had before. The problem is reintroduced by
> the 'fix' to a comment a couple of posts above, about removing the specification
> of CFLAGS and CXXFLAGS for the 'install' pass. 

For this package, avoiding double compiling is more important.
So if there is no other easy way to avoid this, please readd
CFLAGS and CXXFLAGS on %install stage (and leave a comment
about this on spec file)

By the way, again, doesn't scons have the option like
"--skip-build", which most "setup.py" scripts 
written in python have?
Comment 42 Paul Gregory 2007-01-18 05:40:17 EST
(In reply to comment #41)
> For this package, avoiding double compiling is more important.
> So if there is no other easy way to avoid this, please readd
> CFLAGS and CXXFLAGS on %install stage (and leave a comment
> about this on spec file)
> 
> By the way, again, doesn't scons have the option like
> "--skip-build", which most "setup.py" scripts 
> written in python have?
No, SCons is a very different tool to the Python packaging tools. Basically it
builds a complete dependency tree. The install depends on the build, and if the
build options change, the build has to be redone. 

There are actually two options.

1) As mentioned, replace the CFLAGS and CXXFLAGS settings for the install phase
to ensure that the build options are identical.
2) Skip the 'build' phase altogether, the dependency setup for the 'install'
phase will ensure it builds before installing.

Whichever is most appropriate, the result will be the same.

Paul Gregory
Comment 43 Mamoru TASAKA 2007-01-18 06:24:27 EST
Well, anyway please upload a new spec and srpm so
that I can check it again.

Also, please read my comment 38. This is required for
getting sponsored.
Comment 44 Tobias Sauerwein 2007-01-18 11:50:18 EST
Here is the most recent spec and srpm. 

http://www.cgtobi.de/aqsis/aqsis.spec
http://www.cgtobi.de/aqsis/aqsis-1.2.0-0.2.alpha2.src.rpm

I will send in another review request soon and let you know about it.
Comment 45 Mamoru TASAKA 2007-01-18 12:06:42 EST
Umm..

> ExcludeArch: x86_64
So the suggestion by Nicolas on comment 34 and comment 35 are
gone? Nicolas says that with a patch this package can be
build on x86_64.

And.. please increase the release, keep the order for Changelog!!
Decreasing release number during review make it hard for
reviewers to check what is actually changed.

Please check the suggestion by Nicolas and resubmit a new spec/srpm,
with keeping release order.
Comment 46 Mamoru TASAKA 2007-01-18 12:55:52 EST
(Just renaming...)
Comment 47 Tobias Sauerwein 2007-01-18 13:08:18 EST
As requested, I included Nicolas suggestions. Thanks Nicolas btw.

http://www.cgtobi.de/aqsis/aqsis.spec
http://www.cgtobi.de/aqsis/aqsis-1.2.0-0.7.alpha2.src.rpm
Comment 48 Mamoru TASAKA 2007-01-18 14:11:00 EST
Well, for -0.7:

* Please mark %config as %config(noreplace) unless you have a
  reason you don't want to.

* I checked the tarball insize your srpm (-0.7), however, it differs
  from the one I can get from the URL written as Source0??

tarball in srpm:  size 1012133 md5sum 7d0add63c9cd52ab2c6dbb79a8490d37
tarball from URL: size 1012006 md5sum ed944a63d74a528dc8df91068e63456f
Comment 49 Nicolas Chauvet (kwizart) 2007-01-18 15:33:46 EST
From comment#39 Michael Schwendt  seems to say it is better not to build x86_64
(that what i've understood - i noted i need to submit a bug about this
somewhere). It was also discussed in the aqsis forum...
I'm not sure a bugzilla.redhat.com bug entry can be done when the package is in
review. I've just submitted it on the aqsis bugtracker

@cgtobi :  pleased to help!
May i ask if neqsus if supposed to be submitted reviewes ?
 (Blender exporter for aqsis).
http://download.aqsis.org/testing/MISC/neqsus/neqsus-2006-10-28.zip

Comment 50 Nicolas Chauvet (kwizart) 2007-01-18 18:30:57 EST
Don't forget to add: in build step...
libdir=%{_libdir} 
Comment 51 Tobias Sauerwein 2007-01-19 15:01:27 EST
Another update.

RE %config(noreplace)
As we agreed in <pre id="comment_text_28">From <a set="yes"
href="#c28">comment#28</a> I leave it as it is.

RE tarball sice
I used a locale SVN tarball. I marked the new RPM's to show that its a SVN
checkout. New tarball will be released soon. (I am upstream)

RE libdir
Its included now.
I also excluded x86_64 since we agreed internally to leave that for the next
release.

http://www.cgtobi.de/aqsis/aqsis.spec
http://www.cgtobi.de/aqsis/aqsis-1.2.0-0.8.svn738.src.rpm

@kwizart: The problem with packaging Neqsus AKA BtoR si that it relays on cgkit
which isn't available as RPM, so this may become tricky.
Comment 52 Mamoru TASAKA 2007-01-20 20:38:50 EST
Umm...
Any need of using svn version tarball? If there is any strong
reason to use svn version, please tell me the reason and
write where I can get svn source.
Otherwise please use the tarball which are available from
some URL until this review is completed.
Comment 53 Tobias Sauerwein 2007-01-20 20:46:04 EST
A seriouse bug was fixed, so I choose to use a SVN checkout instead of the tarball. As I said a new tarball 
will soon be released. Of course you can get the SVN from sourceforge.net/projects/aqsis .
Comment 54 Tobias Sauerwein 2007-01-21 06:15:04 EST
@Mamoru: Here is my other review request
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=223657
Comment 55 Mamoru TASAKA 2007-01-21 13:12:09 EST
Well, I tried to svn source (rev. 738) and the source
coincides. Now for aqsis it is okay.

So as noted sponsorship is needed. I would sponsor you
when bug 223657 proceeds to some extent enough for me to
accept.
If you don't want to wait, you can still do a pre-review of
other person's review request.
Comment 56 Mamoru TASAKA 2007-02-04 02:00:51 EST
ping?
Comment 57 Tobias Sauerwein 2007-02-04 04:42:32 EST
Sorry, for the delay, but I'm in the middle of my exams. I will soon post new
spec and SRPM for our release candidate or release.
Comment 58 Nicolas Chauvet (kwizart) 2007-02-15 16:27:01 EST
if you can leave x86_64 enabled and use libdir in build step:
scons %{?_smp_mflags} \
		destdir=$RPM_BUILD_ROOT \
                libdir=%{_libdir} \
		install_prefix=%{_prefix} \
		sysconfdir=%{_sysconfdir} \
		no_rpath=true \
		build
I will try to build revision 780 which i supposed to fix the x86_64 bug...
Comment 59 Tobias Sauerwein 2007-02-25 07:47:29 EST
Exams are over and I moved to London. Now I just need to get a fedora machine up and running since the 
opensuse buildserver doesn't seem to do a good job for me.
Comment 61 Tobias Sauerwein 2007-02-28 03:13:22 EST
Cheers Nicolas, that would be very much appreciated since I don't have access to a fedora 5/6 machine 
here.

Thanks in advance.
Comment 62 Mamoru TASAKA 2007-02-28 10:39:42 EST
Well, as this is a sponsor-needed ticket, I want to
check one more review request. So would you update
bug 223657?

By the way, who is the _first_ people who will maintain this
package (and bug 223657)?

Note: now I am trying to update my rawhide system and I expect
that this may take long because perhaps around 300 update packages
are released...
Comment 63 Mamoru TASAKA 2007-03-01 12:18:23 EST
Well, for 1.2.0-1:

* rpmlint
  - is not silent. The followings cannot be ignored.
---------------------------------------------------------------
W: aqsis-debuginfo spurious-executable-perm
/usr/src/debug/aqsis-1.2.0/build/thirdparty/pdiff/LPyramid.h
E: aqsis-debuginfo wrong-script-end-of-line-encoding
/usr/src/debug/aqsis-1.2.0/build/thirdparty/pdiff/LPyramid.h
E: aqsis-debuginfo script-without-shebang
/usr/src/debug/aqsis-1.2.0/build/thirdparty/pdiff/LPyramid.cpp
E: aqsis-debuginfo wrong-script-end-of-line-encoding
/usr/src/debug/aqsis-1.2.0/build/thirdparty/pdiff/LPyramid.cpp
---------------------------------------------------------------
    Permission issue. Please change to 0644.

* Directory ownership issue
  - The following directories are not owned by any package.
---------------------------------------------------------------
/etc/aqsis/
---------------------------------------------------------------

* File conflict
  - I cannot upgrade to 1.2.0-1 (So I have not checked
    1.2.0-1 with this version actually installed).

---------------------------------------------------------------
Dependencies Resolved

=============================================================================
 Package                 Arch       Version          Repository        Size 
=============================================================================
Updating:
 aqsis                   i386       1.2.0-1.fc7      LOCAL             1.9 M
 aqsis-data              i386       1.2.0-1.fc7      LOCAL              14 k
 aqsis-devel             i386       1.2.0-1.fc7      LOCAL              29 k

Transaction Summary
=============================================================================
Install      0 Package(s)         
Update       3 Package(s)         
Remove       0 Package(s)         

Total download size: 1.9 M
Downloading Packages:
Running Transaction Test
Finished Transaction Test


Transaction Check Error:
  file /usr/bin/pdiff from install of aqsis-1.2.0-1.fc7 conflicts with file from
package a2ps-4.13b-61.fc7

Error Summary
-------------
------------------------------------------------------------------
Comment 64 Nicolas Chauvet (kwizart) 2007-03-01 13:35:34 EST
This make me wonder!?!:
a2ps.x86_64                              4.13b-57.fc6.3         installed       
Matched from:
/usr/bin/pdiff

aqsis.x86_64                             1.2.0-1.kwizart.fc6    installed       
Matched from:
/usr/bin/pdiff

I wonder why this package also bundle it! Anyway this mean that i do not need to
enable pdiff (perceptual diff) third part inside aqsis... I also wonder if a2ps
should bundle it or requires package reviewed in #223657...
I would do more testing with this version and see if there is some tweak to do.
Preliminary test is good but some errors messages appear while trying to load
textures files , this should be tweak...

About co-maintainership...
http://fedoraproject.org/wiki/Extras/Policy/EncourageComaintainership
I can propose to be primary maintainer of the package. This can be 
Comment 65 Mamoru TASAKA 2007-03-02 09:34:50 EST
It seems that a2ps pdiff (print diff) and bug 223657
PerceptualDiff are of no relation.
Comment 66 Denis Leroy 2007-03-04 06:06:53 EST
I'm also willing to take over as primary maintainer, since i'm already sponsored
it might save some time, but it's totally up to you guys. I'll sign up for
co-maintainance otherwise. I maintain K-3D, a modeler that's pretty useless
without aqsis.
Comment 67 Tobias Sauerwein 2007-03-04 09:31:40 EST
Sure guys, feel free to take it over. I am happy with that, especially since I don't have access to a fedora 
box over here for the next year.

Cheers
Comment 68 Mamoru TASAKA 2007-03-04 09:38:05 EST
Well, so please someone fix the issue on comment 63?
Especially, how do you want to solve the file conflict
issue against a2ps?
Comment 69 Tobias Sauerwein 2007-03-04 10:50:13 EST
pdiff shouldn't be included in the RPM at all. That's why it is is put into a seperate package. See bug 
223657
Comment 70 Nicolas Chauvet (kwizart) 2007-03-04 10:56:01 EST
The solution to comment to #63 would be to disable this thid party package and
leave https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=223657 control the
name change. PerceptualDiff isn't required so it do not depend of this. But some
study must be done that if package a2ps is installed, it will take Printdiff in
place of Perceptual diff?

This is no replace because of
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=167147#c27
%config %{_sysconfdir}/%{name}/aqsisrc

http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis.spec
http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis-1.2.0-3.kwizart.fc6.src.rpm
Summary: Open source RenderMan-compliant 3D rendering solution

@Denis
Thx you for your help.I can also import it since Mamoru Tasaka has sponsored me.
Since this is your first message on this review (as far as i know) i would
prefer to be the first maintainer for now. So you are welcome to co-maintain it
with me (and tobias?)

@Tobias
Would you like someone to work on PerceptualDiff review if you cannot do this
for now?
Comment 71 Tobias Sauerwein 2007-03-04 10:59:41 EST
@Nicolas 
This would be highly appreciated. If you want to go for it, feel free to do so.
Comment 72 Mamoru TASAKA 2007-03-04 11:06:29 EST
(In reply to comment #70)
 
> This is no replace because of
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=167147#c27
> %config %{_sysconfdir}/%{name}/aqsisrc
I don't mention about noreplace issue.
What I said is that the directory %{_sysconfdir}/%{name} is not
owned by this package.
Comment 73 Nicolas Chauvet (kwizart) 2007-03-04 11:28:34 EST
http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis.spec
http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis-1.2.0-4.kwizart.fc6.src.rpm
Summary: Open source RenderMan-compliant 3D rendering solution

Ok sorry to miss this! 
Comment 74 Mamoru TASAKA 2007-03-04 12:27:01 EST
Okay. 

-----------------------------------------------------
  This package (aqsis) is APPROVED by me.
-----------------------------------------------------
Comment 75 Mamoru TASAKA 2007-03-04 12:30:38 EST
One note:
Currently I cannot access to
http://download.aqsis.org/stable/source/tar/aqsis-1.2.0.tar.gz
Comment 76 Nicolas Chauvet (kwizart) 2007-03-04 12:34:40 EST
Yes, i also have this problem... This may prevent the build to success...
As i remember the timestramp should be good (wget -N ). The URL also but it
fails sometime...
I may need to download from another place if it's really cause failures...
Comment 77 Nicolas Chauvet (kwizart) 2007-03-04 14:22:26 EST
New Package CVS Request
=======================
Package Name: aqsis
Short Description: Open source RenderMan-compliant 3D rendering solution
Owners: kwizart@gmail.com denis@poolshark.org
Branches: FC-5 FC-6 devel
InitialCC: cgtobi@gmail.com
Comment 78 Kevin Fenzi 2007-03-04 14:24:37 EST
Setting fedora-cvs for kwizart per irc conversation. 
Comment 79 Leon Tony Atkinson 2007-03-05 07:29:09 EST
(In reply to comment #75)
> One note:
> Currently I cannot access to
> http://download.aqsis.org/stable/source/tar/aqsis-1.2.0.tar.gz

Our download site is down at the moment (hardware issue), though should be
online again later this evening... apologies for the inconvenience but I will
keep you posted.
Comment 80 Leon Tony Atkinson 2007-03-05 07:36:18 EST
(In reply to comment #77)
> New Package CVS Request
> =======================
> Package Name: aqsis
> Short Description: Open source RenderMan-compliant 3D rendering solution
> Owners: kwizart@gmail.com denis@poolshark.org
> Branches: FC-5 FC-6 devel
> InitialCC: cgtobi@gmail.com

Thanks for offering to maintain this package guys, most appreciated!

Being a co-ordinator of our packaging efforts (Windows, Linux and OS X) would it
be possible to add me to the 'CC' list of this too, along with Tobi?

Many thanks in advance.
Comment 81 Leon Tony Atkinson 2007-03-05 07:43:51 EST
(In reply to comment #76)
> Yes, i also have this problem... This may prevent the build to success...
> As i remember the timestramp should be good (wget -N ). The URL also but it
> fails sometime...
> I may need to download from another place if it's really cause failures...
> 

In the (hopefully rare) event of our download site being offline, you can use
SourceForge as an alternate source for the 'Official' tarball(s)...

http://downloads.sourceforge.net/aqsis/aqsis-1.2.0.tar.gz

Always good to have a contingency plan.  ;-)
Comment 82 Leon Tony Atkinson 2007-03-05 12:09:46 EST
(In reply to comment #79)
> (In reply to comment #75)
> > One note:
> > Currently I cannot access to
> > http://download.aqsis.org/stable/source/tar/aqsis-1.2.0.tar.gz
> 
> Our download site is down at the moment (hardware issue), though should be
> online again later this evening... apologies for the inconvenience but I will
> keep you posted.

Our download site is now (back) online...

http://download.aqsis.org

Enjoy!
Comment 83 Nicolas Chauvet (kwizart) 2007-03-05 16:32:48 EST
New Package CVS Request
=======================
Package Name: aqsis
Short Description: Open source RenderMan-compliant 3D rendering solution
Owners: kwizart@gmail.com denis@poolshark.org
Branches: FC-5 FC-6 devel
InitialCC: cgtobi@gmail.com latkinson@aqsis.org pgregory@aqsis.org

I've missed to include aqsis team for cc
Comment 84 Warren Togami 2007-03-06 13:14:53 EST
Please be sure those members have matching Bugzilla accounts.

done
Comment 85 Mamoru TASAKA 2007-04-09 10:54:07 EDT
Does someone know what is happening on PerceptualDiff review request
(bug 223657)?
Comment 86 Nicolas Chauvet (kwizart) 2007-04-09 11:13:22 EDT
I suppose him to be away from a fedora machine, as i understood...
I will take a look to the review this week...
Comment 87 Nicolas Chauvet (kwizart) 2009-07-09 10:10:06 EDT
Package Change Request
=======================
Package Name: aqsis
Owners: kwizart
New Branches: EL-5
Comment 88 Jason Tibbitts 2009-07-09 23:38:38 EDT
CVS done.

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