Bug 167147

Please give actual URLs to the SRPM and spec file, not just filenames.

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

Description: Aqsis is a high quality, RenderMan-compliant, 3D rendering solution.

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.

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?)

No feedback from submitter for more than 5 months.

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.

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.

*** Bug 202946 has been marked as a duplicate of this bug. ***

(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.

Moreover, if this is your first submission, it should also block bug 177841
(FE-NEEDSPONSOR)

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.

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

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?

(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

> 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.

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.

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.

> The reason is we can't review
> a package whose source cannot be verified upstream.

What exactly do you mean by 'verified upstream'???

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

(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.

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.

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.

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.

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

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).

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.

(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....

(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.

I confirm it built on x86_64 with a patch discussed here:
http://www.aqsis.org/xoops/modules/newbb/viewtopic.php?topic_id=1394&forum=3

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

(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.

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.

Here is the latest update. according to rpmlint the RPM's are fine now. I hope
you agree with me on this.

http://www.cgtobi.de/aqsis/aqsis.spec
http://www.cgtobi.de/aqsis/aqsis-1.2.0-0.5.alpha2.i386.rpm
http://www.cgtobi.de/aqsis/aqsis-1.2.0-0.5.alpha2.src.rpm
http://www.cgtobi.de/aqsis/aqsis-data-1.2.0-0.5.alpha2.i386.rpm
http://www.cgtobi.de/aqsis/aqsis-devel-1.2.0-0.5.alpha2.i386.rpm

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.

About debuginfo rpmlint errors,
you can take a look on this spec:

SPEC
http://kwizart.free.fr/fedora/SPECS/aqsis.spec

(In reply to comment #35)
> SPEC
> http://kwizart.free.fr/fedora/SPECS/aqsis.spec

I will check this spec file.

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.

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
------------------------------------------------------------

> 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.

(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

(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?

(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

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.

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.

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.

(Just renaming...)

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

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

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

Don't forget to add: in build step...
libdir=%{_libdir} 

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.

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.

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 .

@Mamoru: Here is my other review request
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=223657

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.

ping?

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.

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...

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.

http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis.spec
http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis-1.2.0-1.kwizart.fc6.src.rpm

Buildlog:
http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis-1.2.0_2.log
http://kwizart.free.fr/fedora/6/testing/aqsis/aqsis-1.2.0.log

I enabled x86_64 since a proper solution seems to be found. See:
http://www.aqsis.org/xoops/modules/newbb/viewtopic.php?topic_id=1394&forum=3

I can provides co-maintainship with Tobias if needed...

Cheers Nicolas, that would be very much appreciated since I don't have access to a fedora 5/6 machine 
here.

Thanks in advance.

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...

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
-------------
------------------------------------------------------------------

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 

It seems that a2ps pdiff (print diff) and bug 223657
PerceptualDiff are of no relation.

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.

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

Well, so please someone fix the issue on comment 63?
Especially, how do you want to solve the file conflict
issue against a2ps?

pdiff shouldn't be included in the RPM at all. That's why it is is put into a seperate package. See bug 
223657

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?

@Nicolas 
This would be highly appreciated. If you want to go for it, feel free to do so.

(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.

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! 

Okay. 

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

One note:
Currently I cannot access to
http://download.aqsis.org/stable/source/tar/aqsis-1.2.0.tar.gz

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...

New Package CVS Request
=======================
Package Name: aqsis
Short Description: Open source RenderMan-compliant 3D rendering solution
Owners: kwizart denis
Branches: FC-5 FC-6 devel
InitialCC: cgtobi

Setting fedora-cvs for kwizart per irc conversation. 

(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.

(In reply to comment #77)
> New Package CVS Request
> =======================
> Package Name: aqsis
> Short Description: Open source RenderMan-compliant 3D rendering solution
> Owners: kwizart denis
> Branches: FC-5 FC-6 devel
> InitialCC: cgtobi

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.

(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.  ;-)

(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!

New Package CVS Request
=======================
Package Name: aqsis
Short Description: Open source RenderMan-compliant 3D rendering solution
Owners: kwizart denis
Branches: FC-5 FC-6 devel
InitialCC: cgtobi latkinson pgregory

I've missed to include aqsis team for cc

Please be sure those members have matching Bugzilla accounts.

done

Does someone know what is happening on PerceptualDiff review request
(bug 223657)?

I suppose him to be away from a fedora machine, as i understood...
I will take a look to the review this week...

Package Change Request
=======================
Package Name: aqsis
Owners: kwizart
New Branches: EL-5

CVS done.