Bug 381241 - Review Request: ncl - NCAR Command Language and NCAR Graphics
Review Request: ncl - NCAR Command Language and NCAR Graphics
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On: 257741
Blocks: 434349
  Show dependency treegraph
 
Reported: 2007-11-13 17:30 EST by Orion Poplawski
Modified: 2008-04-27 09:58 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-04-27 09:58:53 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ed: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
use datadir for noarch files and other fixes (7.11 KB, patch)
2008-02-01 19:54 EST, Patrice Dumas
no flags Details | Diff
corresponding patch for Site.local, also use more rpm macros (611 bytes, patch)
2008-02-01 19:54 EST, Patrice Dumas
no flags Details | Diff
build file patch to install noarch files in NCARG_ROOT/share (5.16 KB, patch)
2008-02-01 19:56 EST, Patrice Dumas
no flags Details | Diff
don't have install depend on target (429 bytes, patch)
2008-02-01 20:01 EST, Patrice Dumas
no flags Details | Diff
specific build target are put after generic ones (1.02 KB, patch)
2008-02-01 20:02 EST, Patrice Dumas
no flags Details | Diff
link against netcdff for fortran linking (586 bytes, patch)
2008-02-01 20:04 EST, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Orion Poplawski 2007-11-13 17:30:14 EST
Spec URL: http://www.cora.nwra.com/~orion/fedora/ncl.spec
SRPM URL: http://www.cora.nwra.com/~orion/fedora/ncl-5.0.0-1.fc8.src.rpm
Description:
The NCAR Command Language (NCL) is a free interpreted language designed
specifically for scientific data processing and visualization. NCL has robust
file input and output. It can read and write netCDF-3, netCDF-4 classic (as
of version 4.3.1), HDF4, binary, and ASCII data, and read HDF-EOS2, GRIB1 and
GRIB2 (as of version 4.3.0). The graphics are world class and highly
customizable.

As of version 5.0.0, NCL and NCAR Graphics are released as one package.

The software comes with a couple of useful command line tools:

  * ncl_filedump - prints the contents of supported files (netCDF, HDF,
    GRIB1, GRIB2, HDF-EOS2, and CCM History Tape)
  * ncl_convert2nc - converts one or more GRIB1, GRIB2, HDF and/or HDF-EOS
    files to netCDF formatted files.



NOTE - this package will replace the current ncarg package in Fedora that I
maintain.
Comment 1 Patrice Dumas 2007-11-14 06:37:33 EST
rpmlint says the following (certainly) relevant comments:

ncl.i386: E: executable-sourced-script /etc/profile.d/ncarg.csh 0755
ncl.i386: W: devel-file-in-non-devel-package /usr/lib/ncarg/ncarg/graphcaps/aed.a
ncl.i386: E: executable-sourced-script /etc/profile.d/ncarg.sh 0755
ncl.i386: W: non-standard-group Engineering/Scientific
ncl.src: W: non-standard-group Engineering/Scientific
ncl.src: W: strange-permission ncarg.csh 0755
ncl.src: W: strange-permission ncarg.sh 0755

Group could be Applications/Engineering.
Comment 2 Orion Poplawski 2007-11-14 18:04:18 EST
Thanks, should be fixed now.

* Wed Nov 14 2007 - Orion Poplawski <orion@cora.nwra.com> - 5.0.0-2
- Fixup profile.d script permissions, Group, move aed.a to devel

http://www.cora.nwra.com/~orion/fedora/ncl-5.0.0-2.fc8.src.rpm
http://www.cora.nwra.com/~orion/fedora/ncl.spec
Comment 3 Jason Tibbitts 2007-11-17 13:39:45 EST
The remaining rpmlint complaints are:
  ncl.x86_64: W: non-conffile-in-etc /etc/profile.d/ncarg.csh
  ncl.x86_64: W: non-conffile-in-etc /etc/profile.d/ncarg.sh
which are both OK as I understand things.

I know this isn't going to be a pretty package, what with needing both imake and
fortran to build, but the end result doesn't seem terribly bad.

Some of the binaries seem destined to conflict with something (fontc, med,
psplit, etc.) but this package has been around for so long that it would be
pointless to think of renaming any of them.

Now, on to the stuff in %_libdir:

First, why is it all in %_libdir/ncarg/ncarg?  Why not just one ncarg?

What are the .o files in %_libdir/ncarg/ncarg/robj for?

Your -devel package should provide ncl-static = %{version}-%{release}.

But really, for a crufty piece of software like this, this package isn't bad.
Comment 4 Patrice Dumas 2007-11-17 16:02:32 EST
There are some items I don't understand in Site.local. Why is there
#define BuildUdunits FALSE
#define BuildV5D TRUE
#define BuildNetCDF4 TRUE

Also, I haven't verified, but in the README it is said that the 
CcOptions,... can be redefined in Site.local.

Another thing is that it seems to me that it would be better
to set NCARG_ROOT and NCARG_LIB to the right default in 
config/Project or config/Site.local instead of using 
environment variables.

Comment 5 Patrice Dumas 2007-11-17 16:18:42 EST
I can do patches to do what I propose.

Also looks like imake is not used, instead they use ymake,
though this may be the same?
Comment 6 Orion Poplawski 2007-11-18 12:30:07 EST
(In reply to comment #3)
> Now, on to the stuff in %_libdir:
> 
> First, why is it all in %_libdir/ncarg/ncarg?  Why not just one ncarg?

We put the .a in %_libdir/ncarg to avoid (possible?) conflicts.  Once that has
been done, you end up with ncarg/ncarg, and it's much too much of a mess to try
to change.

> What are the .o files in %_libdir/ncarg/ncarg/robj for?

Looks like they are various optional features that can be turned on through
flags in ncargfcc, etc. when compiling projs.  I've moved the -devel package.

> Your -devel package should provide ncl-static = %{version}-%{release}.

Correct, done.

> But really, for a crufty piece of software like this, this package isn't bad.

Thanks :-)
Comment 7 Patrice Dumas 2007-12-13 12:31:53 EST
What about my comments?

Also I have something really strange, there is no ncl executable in
the rpm I rebuilded??? Do you see the same? Did I messed something?
I will investigate in any case.
Comment 8 Orion Poplawski 2007-12-13 14:46:01 EST
(In reply to comment #4)
> There are some items I don't understand in Site.local. Why is there
> #define BuildUdunits FALSE

Changed.

> #define BuildV5D TRUE

Changed.

> #define BuildNetCDF4 TRUE

Changed.

> Also, I haven't verified, but in the README it is said that the 
> CcOptions,... can be redefined in Site.local.

Yeah, but you end up with a lot of warnings about redefining macros during the
dependency generation stage.  I don't like lots of spurious warnings.
  
> Another thing is that it seems to me that it would be better
> to set NCARG_ROOT and NCARG_LIB to the right default in 
> config/Project or config/Site.local instead of using 
> environment variables.

I found it impossible to do because of how the install system works.

Currently testing some patches, should have more soon...
Comment 9 Patrice Dumas 2007-12-13 15:08:55 EST
(In reply to comment #7)
> Also I have something really strange, there is no ncl executable in
> the rpm I rebuilded??? Do you see the same? Did I messed something?
> I will investigate in any case.

I have certainly found the error:

gcc -ansi -fPIC -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -O 
-I../../.././include -I/usr/include/netcdf-3  -DLinux -DMAJOR=2 -DUSE_NETCDF4
-DBuildHDFEOS -DBuildGRIB2    -D_POSIX_SOURCE -D_XOPEN_SOURCE -DByteSwapped
-DNeedFuncProto    -c -o scanner.o scanner.c
ncl.l: In function '_NclParseString':
ncl.l:76: warning: implicit declaration of function 'yyparse'
ncl.l: In function '_NclResetScanner':
ncl.l:199: warning: implicit declaration of function 'isatty'
ncl.l: In function 'yylex':
ncl.l:273: warning: implicit declaration of function '_NclCallPromptFunc'
ncl.l:805: warning: ignoring return value of 'fwrite', declared with attribute
warn_unused_result
lex.yy.c: In function 'yy_get_next_buffer':
lex.yy.c:2099: error: 'yy_current_buffer' undeclared (first use in this function)
lex.yy.c:2099: error: (Each undeclared identifier is reported only once
lex.yy.c:2099: error: for each function it appears in.)
make[4]: *** [scanner.o] Error 1

and make happily continues...
Comment 10 Orion Poplawski 2007-12-13 15:27:24 EST
Definitely what I hate about the ncarg/ncl build system.  I've fixed that error
and am on to another.  Will post when I have a working version.
Comment 11 Orion Poplawski 2007-12-14 11:33:23 EST
This should actually build ncl:

http://www.cora.nwra.com/~orion/fedora/ncl-5.0.0-4.fc8.src.rpm
http://www.cora.nwra.com/~orion/fedora/ncl.spec

It also uses ATLAS instead of the packaged blas and lapack libraries but hasn't
been tested.  I'm getting some undefined symbol errors that I need to look into too.
Comment 12 Orion Poplawski 2007-12-14 18:36:12 EST
This builds wrapit77:

http://www.cora.nwra.com/~orion/fedora/ncl-5.0.0-5.fc8.src.rpm
http://www.cora.nwra.com/~orion/fedora/ncl.spec

I think that's it for the major build issues.
Comment 13 Patrice Dumas 2008-02-01 19:53:06 EST
I made some work on ncl to correct some issues I see. noarch
stuff isn't in datadir, paths could be fixed. I ran into
other issues while doing that. Here are the patches.
Comment 14 Patrice Dumas 2008-02-01 19:54:08 EST
Created attachment 293771 [details]
use datadir for noarch files and other fixes
Comment 15 Patrice Dumas 2008-02-01 19:54:49 EST
Created attachment 293772 [details]
corresponding patch for Site.local, also use more rpm macros
Comment 16 Patrice Dumas 2008-02-01 19:56:50 EST
Created attachment 293773 [details]
build file patch to install noarch files in NCARG_ROOT/share

I think that this patch could go upstream.
Comment 17 Patrice Dumas 2008-02-01 20:01:11 EST
Created attachment 293774 [details]
don't have install depend on target

I am not sure that it is right, but otherwise the in source libraries
have ranlib rerun on them just before they are installed, causing
the binaries that depend on these libraries to be 
relinked before installation.
Comment 18 Patrice Dumas 2008-02-01 20:02:59 EST
Created attachment 293776 [details]
specific build target are put after generic ones

This allows to have the 'all' target be the first one,
instead of one of the scripts target, such that the
default is to remake all the scripts.
Comment 19 Patrice Dumas 2008-02-01 20:04:17 EST
Created attachment 293777 [details]
link against netcdff for fortran linking
Comment 20 Orion Poplawski 2008-02-06 16:40:25 EST
Patrice -

  Thanks for all your hard work here!  I've applied the patches.

http://www.cora.nwra.com/~orion/fedora/ncl-5.0.0-8.fc8.src.rpm
http://www.cora.nwra.com/~orion/fedora/ncl.spec

* Wed Feb  6 2008 - Orion Poplawski <orion@cora.nwra.com> - 5.0.0-8
- Move examples into separate sub-package

* Fri Feb  1 2008 - Patrice Dumas <pertusus@free.fr> - 5.0.0-7
- put noarch files in datadir
- avoid compilation in %%install

* Mon Jan 14 2008 - Orion Poplawski <orion@cora.nwra.com> - 5.0.0-6
- Make BR hdf-devel >= 4.2r2.

Comment 21 Patrice Dumas 2008-02-09 08:40:20 EST
A remaining issue:

I cannot use the source url, it leads to unauthorized.

Minor issues:

The Version isn't in sync with %changelog.

The buildrequires imake isn't needed (unless I am wrong).

Missing dots at the end of devel and examples %descriptions.

I suggest calling Site.local along ncl-Site.local instead.

I also suggest shortening a bit the devel package summary. 
Comment 22 Orion Poplawski 2008-02-09 11:53:32 EST
You must register for an account at http://www.earthsystemgrid.org/ before
downloading the source.  I complained about this on the NCL list, but they feel
that their government contracts dictate that they try to collect usage
statistics and they feel this is the only option available for them.  Made a
note of this in the spec.

Upped the release number.

imake provides makedepend which is used.

added dots

Fixed up devel summary/description

Not sure I understand about Site.local.  Seems fine the way it is.

Comment 23 Patrice Dumas 2008-02-09 13:14:05 EST
(In reply to comment #22)
> You must register for an account at http://www.earthsystemgrid.org/ before
> downloading the source.  I complained about this on the NCL list, but they feel
> that their government contracts dictate that they try to collect usage
> statistics and they feel this is the only option available for them.  Made a
> note of this in the spec.

This is really, really (really) annoying. Moreover it doesn't make 
sense since anybody can repost the archive and deployments are
better done through packages. I personnally don't want to approve a 
package with such a restriction done upstream.

Otherwise I think that the package is acceptable.

> imake provides makedepend which is used.

Not a blocker, but a comment would be nice.
 
> Not sure I understand about Site.local.  Seems fine the way it is.

My point is that Site.local is not really specific of ncl, one
could imagine that a file with the same name is in another package.
Disambiguating the same in SOURCES may be useful in settings where
the same SOURCES directory is used for all packages.

As I said, only a suggestion.
Comment 24 Orion Poplawski 2008-02-18 17:00:40 EST
(In reply to comment #23)
> (In reply to comment #22)
> 
> > imake provides makedepend which is used.
> 
> Not a blocker, but a comment would be nice.

Done.
  
> > Not sure I understand about Site.local.  Seems fine the way it is.
> 
> My point is that Site.local is not really specific of ncl, one
> could imagine that a file with the same name is in another package.
> Disambiguating the same in SOURCES may be useful in settings where
> the same SOURCES directory is used for all packages.
> 
> As I said, only a suggestion.

Ah, I always forget about shared SOURCES directories.  Changed.

http://www.cora.nwra.com/~orion/fedora/ncl-5.0.0-9.fc8.src.rpm
http://www.cora.nwra.com/~orion/fedora/ncl.spec

* Mon Feb 18 2008 - Orion Poplawski <orion@cora.nwra.com> - 5.0.0-9
- Rename Site.local to Site.local.ncl
- Add comment for imake BR

Thanks again Patrice for your work.

Anyone else out there willing to approve a package as it is (with having to
register to download source)?
Comment 25 Ed Hill 2008-02-26 23:04:19 EST
After reviewing all the comments here it seems that Patrice has done a
very thorough review.  The only thing missing seems to be the "source 
matches upstream" check which was omitted due to Pat's (understandable) 
stance on the annoying must-register-to-download primary web site.  After
registering I can confirm: 

 + source matches upstream with sha1sum:
    eca618a1179356e2950608da0e9ca210f1caa1a8  ncl_ncarg_src-5.0.0.tar.gz
    eca618a1179356e2950608da0e9ca210f1caa1a8  ../ncl_ncarg_src-5.0.0.tar.gz
 + specfile is clean and legible
 + license appears to be correct
 + builds in mock on F8 x86_64

Since there appear to be no blockers and since Patrice has already done a 
very thorough review, I'd like to APPROVE this package.
Comment 26 Orion Poplawski 2008-02-27 16:51:35 EST
New Package CVS Request
=======================
Package Name: ncl
Short Description: NCAR Command Language and NCAR Graphics
Owners: orion
Branches: F-8 F-7 EL-5
InitialCC: 
Comment 27 Kevin Fenzi 2008-02-28 14:51:55 EST
cvs done.
Comment 28 Patrice Dumas 2008-04-27 09:58:53 EDT
This is built, closing.

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