I am taking this review. Further details will come soon.

Hi Mark,

Maybe I can give some pointers:

[1] The spec *Source0:* URL can be simplified: `%{url}archive/main.tar.gz`

[2] The spec %files section is very long, it may contain wildcards (*). So for example, updating the following:

...

%{_datadir}/%{name}/data/KOH.cti
%{_datadir}/%{name}/data/KOH.xml
%{_datadir}/%{name}/data/KOH.yaml
%{_datadir}/%{name}/data/air.cti
%{_datadir}/%{name}/data/air.xml
%{_datadir}/%{name}/data/air.yaml
%{_datadir}/%{name}/data/airNASA9.cti
%{_datadir}/%{name}/data/airNASA9.xml
%{_datadir}/%{name}/data/airNASA9.yaml

%{_datadir}/%{name}/samples/cxx/combustor/combustor.cpp
%{_datadir}/%{name}/samples/cxx/custom/CMakeLists.txt

...

To use wildcards:

...

%{_datadir}/%{name}/data/*.cti
%{_datadir}/%{name}/data/*.xml
%{_datadir}/%{name}/data/*.yaml

%{_datadir}/%{name}/samples/cxx/*/*.cpp
%{_datadir}/%{name}/samples/cxx/*/*.txt

...

When submitting a fedora review you should give the direct path to the srpm and not just the folder.

This allows to make some steps in the review automatic. E.g. in order to help me analyze the package I use as a first stage fedora-review.

Here that fails because:

$ fedora-review -b 2037645
INFO: Processing bugzilla bug: 2037645
INFO: Getting .spec and .srpm Urls from : 2037645
ERROR: 'Cannot find source rpm URL'

(In reply to Christopher Crouse from comment #2)
> Hi Mark,
> 
> Maybe I can give some pointers:
> 
> [1] The spec *Source0:* URL can be simplified: `%{url}archive/main.tar.gz`
> 
> [2] The spec %files section is very long, it may contain wildcards (*). So
> for example, updating the following:
> 
> ...
> 
> %{_datadir}/%{name}/data/KOH.cti
> %{_datadir}/%{name}/data/KOH.xml
> %{_datadir}/%{name}/data/KOH.yaml
> %{_datadir}/%{name}/data/air.cti
> %{_datadir}/%{name}/data/air.xml
> %{_datadir}/%{name}/data/air.yaml
> %{_datadir}/%{name}/data/airNASA9.cti
> %{_datadir}/%{name}/data/airNASA9.xml
> %{_datadir}/%{name}/data/airNASA9.yaml
> 
> %{_datadir}/%{name}/samples/cxx/combustor/combustor.cpp
> %{_datadir}/%{name}/samples/cxx/custom/CMakeLists.txt
> 
> ...
> 
> To use wildcards:
> 
> ...
> 
> %{_datadir}/%{name}/data/*.cti
> %{_datadir}/%{name}/data/*.xml
> %{_datadir}/%{name}/data/*.yaml
> 
> %{_datadir}/%{name}/samples/cxx/*/*.cpp
> %{_datadir}/%{name}/samples/cxx/*/*.txt
> 
> ...

Even simple is just:

%{_datadir}/%{name}

because then both the directory and its content will be added.

(In reply to José Matos from comment #4)
> (In reply to Christopher Crouse from comment #2)
> > Hi Mark,
> > 
> > Maybe I can give some pointers:
> > 
> > [1] The spec *Source0:* URL can be simplified: `%{url}archive/main.tar.gz`
> > 
> > [2] The spec %files section is very long, it may contain wildcards (*). So
> > for example, updating the following:
> > 
> > ...
> > 
> > %{_datadir}/%{name}/data/KOH.cti
> > %{_datadir}/%{name}/data/KOH.xml
> > %{_datadir}/%{name}/data/KOH.yaml
> > %{_datadir}/%{name}/data/air.cti
> > %{_datadir}/%{name}/data/air.xml
> > %{_datadir}/%{name}/data/air.yaml
> > %{_datadir}/%{name}/data/airNASA9.cti
> > %{_datadir}/%{name}/data/airNASA9.xml
> > %{_datadir}/%{name}/data/airNASA9.yaml
> > 
> > %{_datadir}/%{name}/samples/cxx/combustor/combustor.cpp
> > %{_datadir}/%{name}/samples/cxx/custom/CMakeLists.txt
> > 
> > ...
> > 
> > To use wildcards:
> > 
> > ...
> > 
> > %{_datadir}/%{name}/data/*.cti
> > %{_datadir}/%{name}/data/*.xml
> > %{_datadir}/%{name}/data/*.yaml
> > 
> > %{_datadir}/%{name}/samples/cxx/*/*.cpp
> > %{_datadir}/%{name}/samples/cxx/*/*.txt
> > 
> > ...
> 
> Even simple is just:
> 
> %{_datadir}/%{name}
> 
> because then both the directory and its content will be added.

Hi José,

I agree 100%, it was the 3rd suggestion I was going to make, but you started the review.

Regardless, the point I was trying to make is just stating the fact that wildcards are allowed. 

Please continue your review :)

(In reply to Christopher Crouse from comment #5)
> Hi José,
> 
> I agree 100%, it was the 3rd suggestion I was going to make, but you started
> the review.
> 
> Regardless, the point I was trying to make is just stating the fact that
> wildcards are allowed. 
> 
> Please continue your review :)

FWIW I appreciate your comments. And for newcomers all the help is welcome.

More recent spec file syntax is a lot more amenable and not something you necessarily see in the online documentation.
It is not that the documentation is incorrect but some of it is outdated and it does not apply.

So please feel free to comment as you consider adequate. :-)

Thank you both very much already.
I am making the suggested changes to the spec file right now.

Here are revised links for the spec and SRPM files:
Spec URL: https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-35-x86_64/03121441-cantera/cantera.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-35-x86_64/03121441-cantera/cantera-2.6.0-0.3.a4.fc35.src.rpm

I made all the updates suggested so far to the spec file and have updated the spec and SRPM accordingly

Spec URL: https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-35-x86_64/03130228-cantera/cantera.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-35-x86_64/03130228-cantera/cantera-2.6.0-0.3.a4.fc35.src.rpm

Looks much better now!

Some more comments:

- I noticed the changelog is in a nonstandard format. I would recommend using "rpmdev-bumpspec", will handle adding the entry and formatting the entry correctly for you.

  See what the output is of the following command on your spec file:
  $ rpmdev-bumpspec --comment="Foo"

  Note: Might also might want to look into rpmautospec, only once imported. It will make maintaining and writing the *Release* and *%changelog* messages unnecessary in the future.
  Documentation here: https://docs.pagure.org/fedora-infra.rpmautospec/index.html

- I see that you have enabled fedora-review on your COPR repo. These files can help you debug issues when packaging:
  - https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-35-x86_64/03130228-cantera/fedora-review/review.txt
  - https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-35-x86_64/03130228-cantera/fedora-review/licensecheck.txt
  - https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-35-x86_64/03130228-cantera/fedora-review/rpmlint.txt

- I always put some newlines between the macros I use in the %files section. 
  It improves the readability and also a personal preference, for example:

  ...

  %doc %{_mandir}/man1/ctml_writer.1.gz

  %{_bindir}/ck2cti
  %{_bindir}/ck2yaml
  %{_bindir}/cti2yaml
  %{_bindir}/ctml2yaml
  %{_bindir}/ctml_writer

  %{_datadir}/%{name}

  ...

- Lastly, would recommend to do a koji scratch build yourself, and adding the URL in the comments.
  Documentation here: https://docs.fedoraproject.org/en-US/package-maintainers/Using_the_Koji_Build_System/

In addition to Christopher's remark here are some other issues.

You should bump the release number after each change in the review process. It becomes easier for us to see the progress between versions.


The srpm does not build, the root cause is that the binaries are installed in /usr/local/bin and not in /usr/bin as you declare in %files:

...
running install_scripts
Installing ck2cti script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
Installing ck2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
Installing cti2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
Installing ctml2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
Installing ctml_writer script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
...
Processing files: cantera-common-2.6.0-0.3.a4.fc36.x86_64
error: File not found: /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/bin/ck2cti
error: File not found: /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/bin/ck2yaml
error: File not found: /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/bin/cti2yaml
error: File not found: /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/bin/ctml2yaml
error: File not found: /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/bin/ctml_writer


Regarding the spec some other suggestions. Where you have:

%if 0%{?fedora}
BuildRequires:  gcc-fortran
%endif

%if 0%{?rhel}
BuildRequires:  gcc-gfortran
%endif

I would suggest:
%if 0%{?fedora}
BuildRequires:  gcc-fortran
%else
BuildRequires:  gcc-gfortran
%endif

The initial code is correct, of course, but the advantage of my suggestion is that it shows the intent. We see immediately that the purpose is to satisfy the same requirement on different systems.

Another suggestion, that is completely optional, is to avoid to repeat the description text:

%global common_description %{expand: \
 Cantera is a suite of object-oriented software tools for solving problems
 involving chemical kinetics, thermodynamics, and/or transport processes.
 Cantera can be used for simulating time-dependent or steady reactor
 networks and one-dimensional reacting flows. Thermodynamic models for
 ideal gases, aqueous electrolytes, plasmas, and multiphase substances
 are provided.}

And then you can simply replace them in the different descriptions:

%description
%{common_description}

%package common
Summary: Common files needed for all Cantera interfaces
%description common
%{common_description}

This package includes programs for parsing and converting chemical
mechanisms, a set of common mechanism files, and several sample problems.

...

Hi Jose,

Regarding the SRPM not building, this is a known problem for all but one buildroot on Rawhide/F36 and it probably has to do with a problem in the SConstruct file.
The SRPM builds properly under F34, F35.

I have an alternate spec file at https://github.com/mefuller/cantera/blob/copr/rpm/cantera-rawhide.spec which has a modified `%install` section:
```
%install
scons install stage_dir=%{buildroot}
mv %{buildroot}%{_prefix}/local/bin/ck2cti %{buildroot}%{_prefix}/bin/ck2cti
mv %{buildroot}%{_prefix}/local/bin/ck2yaml %{buildroot}%{_prefix}/bin/ck2yaml
mv %{buildroot}%{_prefix}/local/bin/cti2yaml %{buildroot}%{_prefix}/bin/cti2yaml
mv %{buildroot}%{_prefix}/local/bin/ctml2yaml %{buildroot}%{_prefix}/bin/ctml2yaml
mv %{buildroot}%{_prefix}/local/bin/ctml_writer %{buildroot}%{_prefix}/bin/ctml_writer
```

If you can suggest how to check the location of these files and move them only when needed so as to unify the spec files, I would be very appreciative

You can see this bifurcation in the builds at https://copr.fedorainfracloud.org/coprs/fuller/Cantera/build/3121441/ and https://copr.fedorainfracloud.org/coprs/fuller/Cantera/build/3121440/

Sorry, I forgot to add the link to the known problem/open issue with Cantera installing files in the wrong location: https://github.com/Cantera/cantera/issues/1149

The Koji scratch build is at https://koji.fedoraproject.org/koji/taskinfo?taskID=80914079

(In reply to Mark E. Fuller from comment #12)
> Sorry, I forgot to add the link to the known problem/open issue with Cantera
> installing files in the wrong location:
> https://github.com/Cantera/cantera/issues/1149

The guidelines explicitly tell us to test the package in rawhide.


Could you try to see if it works to pass prefix=%{_prefix} directly in the install stage, e.g. in Fedora:

scons install prefix=%{_prefix} stage_dir=%{buildroot}


Another suggestion to shorten the code is to define:
%global scons scons%{?rhel:-3}

and then you can simply use %scons when calling scons regardless of being Fedora or EPEL, assuming naturally that the options are the same, as they seem on a first reading.

I modified the spec locally, and added the following to the %install section, after the `scons install` command:

...

%if 0%{?fedora} > 35
    mv %{buildroot}%{_prefix}/local/bin/ck2cti %{buildroot}%{_prefix}/bin/ck2cti
    mv %{buildroot}%{_prefix}/local/bin/ck2yaml %{buildroot}%{_prefix}/bin/ck2yaml
    mv %{buildroot}%{_prefix}/local/bin/cti2yaml %{buildroot}%{_prefix}/bin/cti2yaml
    mv %{buildroot}%{_prefix}/local/bin/ctml2yaml %{buildroot}%{_prefix}/bin/ctml2yaml
    mv %{buildroot}%{_prefix}/local/bin/ctml_writer %{buildroot}%{_prefix}/bin/ctml_writer
%endif
...

Rawhide fails to build on Arch i686 & armv7hl, however succeeds on the rest.
Interesting thing is, builds on Arch i686 & armv7hl does not require the above workaround, since it installs correctly to /usr/bin.

Koji rawhide scratch build URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=80920650

Arch x86_64:

Installing ck2cti script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
Installing ck2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
Installing cti2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
Installing ctml2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin
Installing ctml_writer script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.x86_64/usr/local/bin

Arch i686:

Installing ck2cti script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.i386/usr/bin
Installing ck2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.i386/usr/bin
Installing cti2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.i386/usr/bin
Installing ctml2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.i386/usr/bin
Installing ctml_writer script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.i386/usr/bin

Arch armv7hl:

Installing ck2cti script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.arm/usr/bin
Installing ck2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.arm/usr/bin
Installing cti2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.arm/usr/bin
Installing ctml2yaml script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.arm/usr/bin
Installing ctml_writer script to /builddir/build/BUILDROOT/cantera-2.6.0-0.3.a4.fc36.arm/usr/bin

Could you, please, add the prefix=%{_prefix} to the scons install call?

Looking into packages that require scons to build I see that pattern.

# repoquery --disablerepo=* --enablerepo=rawhide-source --whatrequires scons
Fedora - Rawhide - Source                           5.7 MB/s | 7.8 MB     00:01    
Last metadata expiration check: 0:00:01 ago on Thu 06 Jan 2022 04:43:03 PM WET.
endless-sky-0:0.9.14-2.fc35.src
lcdtest-0:1.18-27.fc35.src
libnxt-0:0.3-26.fc35.src
mapnik-0:3.1.0-14.fc36.src
rmlint-0:2.10.1-6.fc35.src
sar2-0:2.5.0-4.fc35.src
v8-314-0:3.14.5.10-26.fc35.src
zfs-fuse-0:0.7.2.2-21.fc36.src

Thanks for all the advice - this is extremely helpful.
Also sorry for the delay - it's the weekend in Israel.

Latest koji scratch build with the suggested modifications is at https://koji.fedoraproject.org/koji/taskinfo?taskID=80951521
As you can see, arch s390x, x86_64, ppc64le and aarch64 all failed while i686 and armv7hl successfully built.
All failures were due to the same error as above; adding the prefix=%{_prefix} to the scons install call had no effect

(In reply to Mark E. Fuller from comment #17)
> Thanks for all the advice - this is extremely helpful.
> Also sorry for the delay - it's the weekend in Israel.

There is no problem, Shabbat Shalom. :-)

> Latest koji scratch build with the suggested modifications is at
> https://koji.fedoraproject.org/koji/taskinfo?taskID=80951521
> As you can see, arch s390x, x86_64, ppc64le and aarch64 all failed while
> i686 and armv7hl successfully built.
> All failures were due to the same error as above; adding the
> prefix=%{_prefix} to the scons install call had no effect

I think that you found the culprit. The architectures that failed are where lib is lib64.
From that I suspect that there is some heuristics this is going astray with scons 4.3.

Also for an aesthetic point of view I suggest that the definition of %scons is placed near BuildRequires part.

Regarding scons remember that in order to work in RHEL you should use now %scons (that it will take care of the distinction).
Also to work around this bug in the installer I suggestion the following code that works in all cases:

%install
%scons install prefix=%{_prefix} libdirname=%{_lib} stage_dir=%{buildroot}
# work around a bug in the installer: https://github.com/Cantera/cantera/issues/1149
if [ ! -f %{buildroot}%{_prefix}/bin/ck2cti ]; then
  mv %{buildroot}%{_prefix}/local/bin/* %{buildroot}%{_prefix}/bin
fi

With this code the package builds in x86_64 for both F35 and rawhide and so it should for all the other architectures.

OK, I've implemented the changes and the koji build is at https://koji.fedoraproject.org/koji/taskinfo?taskID=80961831
Everything seems to be working


(Also the COPR builds are running without issue at https://copr.fedorainfracloud.org/coprs/fuller/Cantera/build/3132758/ and https://copr.fedorainfracloud.org/coprs/fuller/Cantera-EL/build/3132755/)

As this is my first package, I also require sponsorship: I commented at https://bugzilla.redhat.com/show_bug.cgi?id=177841 yesterday; was/is there something additional that I need to do?

(In reply to Mark E. Fuller from comment #19)
> OK, I've implemented the changes and the koji build is at
> https://koji.fedoraproject.org/koji/taskinfo?taskID=80961831
> Everything seems to be working
> 
> 
> (Also the COPR builds are running without issue at
> https://copr.fedorainfracloud.org/coprs/fuller/Cantera/build/3132758/ and
> https://copr.fedorainfracloud.org/coprs/fuller/Cantera-EL/build/3132755/)

Please use the direct paths to the srpm and spec files. This allows us to use automatic tools to pick the packages, and in particular fedora-review.
When you do that I will give a more detailed feedback.
 
> As this is my first package, I also require sponsorship: I commented at
> https://bugzilla.redhat.com/show_bug.cgi?id=177841 yesterday; was/is there
> something additional that I need to do?

It would be nice to see some reviews, even if informal, in other entries.

Sorry about that.

Spec URL: https://download.copr.fedorainfracloud.org/results/fuller/Cantera/srpm-builds/03132758/cantera.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/fuller/Cantera/srpm-builds/03132758/cantera-2.6.0-0.4.a4.fc34.src.rpm

I will look into doing some reviews.
To now, I have largely been doing my packaging in COPR to test/prepare for requesting EPEL builds.

Thank you

I've just sponsored Mark into packagers group, so he no longer needs sponsorship.

Running fedora-review identifies the following issues:

- Header files in -devel subpackage, if present.
  Note: cantera-common :
  /usr/share/cantera/samples/cxx/bvp/BoundaryValueProblem.h cantera-common
  : /usr/share/cantera/samples/cxx/kinetics1/example_utils.h
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_devel_packages
- Package does not contain duplicates in %files.
  Note: warning: File listed twice: /usr/share/cantera/doc/LICENSE.txt
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_duplicate_files
- Static libraries in -static or -devel subpackage, providing -devel if
  present.
  Note: Package has .a files: cantera-devel. Does not provide -static:
  cantera-devel.
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries


The first 2 are false-positives, in the sense that the first header file corresponds to an example.
The second case is justified since that is the license file that is present in the documentation.

The third case is true and it should be solved.

Other than that I have no further remarks...

I revised the spec and bumped the distribution: there's now a cantera-static package that includes the two `.a` files such that they need not be downloaded and installed unless specifically desired.

Spec URL: https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-rawhide-x86_64/03163517-cantera/cantera.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/fuller/Cantera/fedora-rawhide-x86_64/03163517-cantera/cantera-2.6.0-0.5.a4.fc36.src.rpm

The scratch build on koji also worked without issue: https://koji.fedoraproject.org/koji/taskinfo?taskID=81227662

Running fedora-review returns the same two false positives as above, but the third issue about static libraries is resolved.

Thank you again for all of your time and patience.

Thank you for all the changes.

I was worried with the python3 sub-package dependencies but I see that they are already met.

The only remaining concern is to have a %check section to ensure that all the tests pass.
Eventually, without checking, I would expect this to be simply:

%check
%scons test


insert this before the %file section.

Sometimes this can require new packages to build but it has the advantage that it catches failures when building the package.
OK, tested locally and it works:

"
print_report(["test_results"], [])

*****************************
***    Testing Summary    ***
*****************************

Tests passed: 1659
Up-to-date tests skipped: 0
Tests failed: 0
*****************************
"

In any case I trust you to try this after importing the package.

So here follows the formal review:

The license is correct, appropriated and included in the package.

The package follows the fedora name scheme and is according to the Fedora guidelines.

So the package is approved.

Thank you Jose, Christopher, and Dominik.
It's very gratifying to be able to contribute a package and I look forward to participating even more in the future.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/Cantera

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/cantera

cantera has been submitted for stable by bodhi

Closing

Thank you everyone again