https://bugzilla.redhat.com/show_bug.cgi?id=1816301
Bug ID: 1816301 Summary: Review Request: openfoam - computational fluid dynamics Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: mark.olesen@esi-group.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://download.copr.fedorainfracloud.org/results/openfoam/openfoam/epel-7-... SRPM URL: https://download.copr.fedorainfracloud.org/results/openfoam/openfoam/epel-7-... Description: herOpenFOAM has an extensive range of features to solve complex fluid flows involving chemical reactions, turbulence and heat transfer, solid dynamics and electromagnetics Fedora Account System Username: openfoam
My first Fedora package, seeking sponsor.
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #1 from mark.olesen@esi-group.com --- FE-NEEDSPONSOR
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |sanjay.ankur@gmail.com Flags| |fedora-review?
--- Comment #2 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Hi Mark,
I'll help review this, and I can sponsor you when it's ready too.
Cheers, Ankur
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
david08741@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |david08741@gmail.com
--- Comment #3 from david08741@gmail.com --- Not a full review, but some comments:
Group: is deprecated, please remove
License should be: GPLv3+ https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses
Name should probably not include the version: Name: openfoam
I am not sure whether this is a MUST, but the release should start at 1 and be bumped whenever you change the spec, without a new release: https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_simpl...
%defattr(-,root,root,-) isn't needed, please remove https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_permissions
I don't think prefix should be set; it is certainly not allowed to use /opt
Buildroot should not be set: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_section...
'%package -n %{name}-examples' -> '%package examples'
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
Benson Muite benson_muite@emailplus.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |benson_muite@emailplus.org
--- Comment #4 from Benson Muite benson_muite@emailplus.org --- Can you check it builds on more than x86, in particular ARM-hfp and AArch64, see https://fedoraproject.org/wiki/Architectures
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #5 from mark.olesen@esi-group.com --- The upstream package is built regularly on ARM64 (with Clang, alternatively with Gcc) - we keep regular contact with ARM people as well. Haven't had access to build with ARM-v7 recently, but it posed no issues a few years back. However, this architecture is not of particular interest any more (performance).
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #6 from Benson Muite benson_muite@emailplus.org --- It should probably build in Copr with current SPEC file if more architecture boxes are checked. Aim is that most users should be able to run the software - portable optimized performance from the RPM is challenging.
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #7 from mark.olesen@esi-group.com --- (In reply to david08741 from comment #3)
Nice to get the reviews - it shows that people care!
Not a full review, but some comments:
Group: is deprecated, please remove
OK for RedHat, probably leave for SuSE.
License should be: GPLv3+ https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses
Interesting, any idea why are they not using or accepting https://spdx.org/licenses/? Thought this would be "standard".
Name should probably not include the version: Name: openfoam
I am not sure whether this is a MUST, but the release should start at 1 and be bumped whenever you change the spec, without a new release:
These are both open to discussion and suggestion about how best to solve. OpenFOAM releases on a 6-month cycle in Jun and Dec, with version (API) denoted as YYMM (eg, 1906, 1912).
Since the API and the internal models most certainly change between these releases, it is fairly standard practice to have multiple versions installed or installable on the system. There are various reasons that this is desirable:
- allows testing, porting of user models to the updated framework - allows back-to-back comparison of simulation results, validation cases etc. - avoids automatic upgrades of major versions. For some industries it is normal to continue with a particular major version for the development lifetime of a product (eg, a vehicle).
The best way that I came up with was to have numbered packages (eg, openfoam1912, openfoam1906, etc) and use a top-level "openfoam" meta-package to define what is the most current release. I guess it could be comparable to having Qt4, Qt5, kde4, kde5, etc, except that the release numbers update every 6 months.
On copr, I'm just now experimenting with using the bugfix (patch) value for the version. The patch value follows a YYMMDD value. This means that the current spec would then have
Name: openfoam1912 Version: 200316 # <- 2020-03-16
The release could than have the usual increment I guess?
%defattr(-,root,root,-) isn't needed, please remove
OK
I don't think prefix should be set; it is certainly not allowed to use /opt
This was also something that was discussed off-line (Fedora and Debian). Need to have isolated, version-specific directories, but using an "alternatives" framework does not appear to be a good fit. We have approximately 300 executables and 160 libraries to deal with. I can't imagine fitting them all into alternatives. Besides which, the choice of which OpenFOAM version to use should be a user choice, not a systems choice.
Did look at trying to drop everything into /usr/lib/, or even install as multi-arch, but without proper guidance decided on /opt for the moment.
I am most certainly open to suggestions.
Buildroot should not be set: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_section...
OK, might have been working from some older docs.
'%package -n %{name}-examples' -> '%package examples'
Nice, much cleaner.
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #8 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- (In reply to mark.olesen from comment #7)
(In reply to david08741 from comment #3)
Nice to get the reviews - it shows that people care!
Not a full review, but some comments:
Group: is deprecated, please remove
OK for RedHat, probably leave for SuSE.
If you intend to use the same spec, you'll have to resort to using conditionals: http://ftp.rpm.org/max-rpm/s1-rpm-specref-conditionals.html
The Fedora conditionals are listed here: https://docs.fedoraproject.org/en-US/packaging-guidelines/DistTag/#_conditio...
I do not know what the conditionals for SuSE are, so you'll have to refer to their documentation.
License should be: GPLv3+ https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses
Interesting, any idea why are they not using or accepting https://spdx.org/licenses/? Thought this would be "standard".
The Red Hat/Fedora legal team oversees the licensing, so we stick to what they suggest :)
Name should probably not include the version: Name: openfoam
I am not sure whether this is a MUST, but the release should start at 1 and be bumped whenever you change the spec, without a new release:
These are both open to discussion and suggestion about how best to solve. OpenFOAM releases on a 6-month cycle in Jun and Dec, with version (API) denoted as YYMM (eg, 1906, 1912).
Since the API and the internal models most certainly change between these releases, it is fairly standard practice to have multiple versions installed or installable on the system. There are various reasons that this is desirable:
- allows testing, porting of user models to the updated framework
- allows back-to-back comparison of simulation results, validation cases etc.
- avoids automatic upgrades of major versions. For some industries it is
normal to continue with a particular major version for the development lifetime of a product (eg, a vehicle).
The best way that I came up with was to have numbered packages (eg, openfoam1912, openfoam1906, etc) and use a top-level "openfoam" meta-package to define what is the most current release. I guess it could be comparable to having Qt4, Qt5, kde4, kde5, etc, except that the release numbers update every 6 months.
On copr, I'm just now experimenting with using the bugfix (patch) value for the version. The patch value follows a YYMMDD value. This means that the current spec would then have
Name: openfoam1912 Version: 200316 # <- 2020-03-16
The release could than have the usual increment I guess?
This is OK. No problem here as long as the various openfoam packages don't conflict with each other.
%defattr(-,root,root,-) isn't needed, please remove
OK
I don't think prefix should be set; it is certainly not allowed to use /opt
This was also something that was discussed off-line (Fedora and Debian). Need to have isolated, version-specific directories, but using an "alternatives" framework does not appear to be a good fit. We have approximately 300 executables and 160 libraries to deal with. I can't imagine fitting them all into alternatives. Besides which, the choice of which OpenFOAM version to use should be a user choice, not a systems choice.
Did look at trying to drop everything into /usr/lib/, or even install as multi-arch, but without proper guidance decided on /opt for the moment.
I am most certainly open to suggestions.
As noted, /opt certainly cannot be used. The best solution here depends on the software. If it's only the binaries we need to worry about, they can simply be suffixed with the version. This also clearly tells the user what version they are using. This is how mpi variants for packages are built, for example. Libraries are much trickier, especially if they use the same names between variants. Are the soname versions different at least? Otherwise even using /opt will only work if the LD_LIBRARY_PATH is updated each time to give the correct version preference, right?
Buildroot should not be set: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_section...
OK, might have been working from some older docs.
'%package -n %{name}-examples' -> '%package examples'
Nice, much cleaner.
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #9 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- The most convenient + cleanest way to enable multiple versions to be installed on a system in parallel would probably be environment modules: https://docs.fedoraproject.org/en-US/packaging-guidelines/EnvironmentModules...
That's how we include MPI based builds for tools. Would you take a look and see what you think?
This is a fairly standard MPI build where an environment module is defined for nest-mpich and nest-openmpi. (Here, they're all built in the same spec file since they're part of the same package) https://src.fedoraproject.org/rpms/nest/blob/master/f/nest.spec
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #10 from david08741@gmail.com --- I agree, modules is probably the best way. That way back-to-back comparisons can use the same script, just different module load before.
There is a documentation for MPI, which includes how to setup module files: https://fedoraproject.org/wiki/Packaging:MPI
Some parts might be useful
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #11 from mark.olesen@esi-group.com --- Currently rebuilding with the various suggested changes, with /usr/lib/openfoam for the common root (instead of /opt).
I would still ideally like to have some more granularity in subpackages, similar to my current section of .deb files. However, I'm not sure if the interdependencies can be properly described in a single .spec file.
The subpackages - myPackage-develop : source code headers and project-specific build scripts. - myPackage-tools : binaries for project-specific build tools. - myPackage-common : version information and share files - myPackage-examples : tutorials
The dependencies:
myPackage - Requires: myPackage-common - Suggests: myPackage-examples
myPackage-develop - Requires: myPackage - Requires: myPackage-tools
myPackage-common - Requires: n/a
myPackage-examples - Requires: n/a
Is it possible to describe these interdependencies in a spec format?
/mark
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #12 from david08741@gmail.com --- (In reply to mark.olesen from comment #11)
Currently rebuilding with the various suggested changes, with /usr/lib/openfoam for the common root (instead of /opt).
I cannot find the relevent guidelines right now, but it would be better if you would put header files in /usr/include/openfoam<v>. Also, you should probably use %{_libdir} rather then hard-coding /usr/lib/ so that /usr/lib64 is used on 64bit systems. Another reason is https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros
I would still ideally like to have some more granularity in subpackages, similar to my current section of .deb files. However, I'm not sure if the interdependencies can be properly described in a single .spec file.
This is no issue. See e.g. https://src.fedoraproject.org/rpms/nest/blob/master/f/nest.spec which does this.
The subpackages
- myPackage-develop : source code headers and project-specific build scripts.
Probably should be called myPackage-devel
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #13 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- (In reply to david08741 from comment #12)
(In reply to mark.olesen from comment #11)
Currently rebuilding with the various suggested changes, with /usr/lib/openfoam for the common root (instead of /opt).
I cannot find the relevent guidelines right now, but it would be better if you would put header files in /usr/include/openfoam<v>. Also, you should probably use %{_libdir} rather then hard-coding /usr/lib/ so that /usr/lib64 is used on 64bit systems. Another reason is https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros
This covers it: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_filesystem_layou...
We adhere quite strictly to the Filesystem Hierarchy Standard. So %{_includedir} is /usr/include, and so on.
I would still ideally like to have some more granularity in subpackages, similar to my current section of .deb files. However, I'm not sure if the interdependencies can be properly described in a single .spec file.
This is no issue. See e.g. https://src.fedoraproject.org/rpms/nest/blob/master/f/nest.spec which does this.
+1 you can make lots of such dependencies in the spec without any trouble.
The subpackages
- myPackage-develop : source code headers and project-specific build scripts.
Probably should be called myPackage-devel
+1
The relevant guideline is here: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #14 from mark.olesen@esi-group.com --- The nest example is very nice. I've been struggling to find a definitive guide of what is actually allowed in a spec file. The rpm.org has things, but seems to be out of date.
Will get back when I have something to re-review.
/mark
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #15 from mark.olesen@esi-group.com --- A quick question for understanding packaging:
If I now package under /usr/lib/openfoam/PKG instead of tossing things into /opt/PKG, rpmlint complains if I do not include /usr/lib/openfoam in the %files list.
But if I package openfoamVER1 as /usr/lib/openfoam/openfoamVER1 and openfoamVER2 as /usr/lib/openfoam/openfoamVER2, who is supposed to "own" the directory? Both, neither?
In a non-RPM world I would think that the last one out should try to remove the directory if possible, but that sounds like a bad hack. Or does one simply state that /usr/lib/openfoam belongs to each package and just rely on the fact that a rmdir of a non-empty directory should fail?
Thanks, /mark
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #16 from david08741@gmail.com --- Why would put them in /usr/lib/openfoam, rather then in /usr/lib/? That would be much nicer, imho, as if you only have one openfoam, it is not unnecessarily one level deeper.
Besides that, you MUST not install in /usr/lib. Header files need to go to %{_includedir}/openfoamVER, libs go to %{_libdir}/openfoamVER etc. See [1] for the specific directories.
If you want to put them in openfoam/openfoamVER/ rather then directly openfoamVER, then there are two options, create a package openfoam-filesystyem that owns the shared openfoam/ directories. The other one is co-owning, as you mentioned. See [2] for more on directory ownership.
[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/RPMMacros/ [2] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directo...
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #17 from mark.olesen@esi-group.com --- Sorry about taking longer to get back to you, caught up in other build issues.
(In reply to david08741 from comment #16)
Why would put them in /usr/lib/openfoam, rather then in /usr/lib/? That would be much nicer, imho, as if you only have one openfoam, it is not unnecessarily one level deeper.
There are a few issues with putting everything into /usr/bin and /usr/lib directly. We have a forest of around 160 libraries and another 300 binaries that most people don't appreciate having dumped into regular locations (cf, https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=918112)
Another vital thing is that we want to support installation of different versions at the same time. This is fairly common in an engineering environment. Someone might need to use openfoam1812 until their current vehicle goes into production, but use openfoam1912 for newer projects. Or just as importantly, might wish to verify how some particular result may be change between versions.
Besides that, you MUST not install in /usr/lib. Header files need to go to %{_includedir}/openfoamVER, libs go to %{_libdir}/openfoamVER etc. See [1] for the specific directories.
This doesn't work very well at all with OpenFOAM. For good or bad, we have a large number of headers and templated code spread across many directories. The homegrown wmake (wrapped make) system creates a web of symlinks for the headers, templates. If we start relocating these out of the source context, we need to rewrite a substantial amount of the wmake system to accommodate this and introduce loads of problems.
If you want to put them in openfoam/openfoamVER/ rather then directly openfoamVER, then there are two options, create a package openfoam-filesystyem that owns the shared openfoam/ directories. The other one is co-owning, as you mentioned. See [2] for more on directory ownership.
An openfoam-filesystem owner package sounds OK. But would prefer to get most of the current package on its way first. Otherwise I fear that we will near leave the starting blocks.
/mark
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #18 from mark.olesen@esi-group.com --- Anything still missing for this to go ahead? The most current set of srpm and specs are listed under
https://download.copr.fedorainfracloud.org/results/openfoam/openfoam/fedora-... https://download.copr.fedorainfracloud.org/results/openfoam/openfoam/fedora-...
/mark
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
Dave Love dave.love@manchester.ac.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dave.love@manchester.ac.uk
--- Comment #19 from Dave Love dave.love@manchester.ac.uk --- I'll comment as I'd previously packaged openfoam and started on making it comply with Fedora-isms.
The approach of putting it in its own tree (under %_libdir) is appropriate, and it isn't a special case in that respect, but I can't see where it actually goes now -- %prefix doesn't seem to be defined. I would expect the source under /usr/src, but I don't know if there's policy on that.
However, the packaging isn't at all right yet. I haven't checked in detail, but I noticed: Fedora doesn't allow conditionals for other distributions (which I think is unfortunate), you need serial and openblas and mpich packages (unless mpich won't work for some reason), I don't understand the bit about CGAL missing in f32.
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #20 from mark.olesen@esi-group.com --- Hi Dave,
(In reply to Dave Love from comment #19)
I'll comment as I'd previously packaged openfoam and started on making it comply with Fedora-isms.
The approach of putting it in its own tree (under %_libdir) is appropriate, and it isn't a special case in that respect, but I can't see where it actually goes now -- %prefix doesn't seem to be defined.
An earlier comment stated that %prefix should not be defined (some policy). I had originally tried to make them relocatable, but that runs counter to what we now have.
I would expect the source under /usr/src, but I don't know if there's policy on that.
Generally true, but the structure of OpenFOAM expects its source under the project-directory. If we put it elsewhere, we would need to patch OpenFOAM like mad and do lots of tests to see that we haven't broken anything. For what it's worth, I've at least split off into proper sub-packages to avoid installing sources unless a '-devel' package is selected.
However, the packaging isn't at all right yet. I haven't checked in detail, but I noticed: Fedora doesn't allow conditionals for other distributions (which I think is unfortunate),
Some sed'ing will work there, but it does seem a bit unfortunate.
you need serial and openblas and mpich packages (unless mpich won't work for some reason),
Doing a proper multibuild (serial and various MPI flavours) is still work-in-progress. I have a proof of concept for adding in additional MPI layers
https://build.opensuse.org/package/show/home:openfoam/openfoam2006mpi
but still haven't worked out a good way to manage the resulting configurations. If we actually need to get multi-build working for this to be a Fedora package, I fear that we will never get finished.
/mark
Product: Fedora Version: rawhide Component: Package Review
mark.olesen@esi-group.com has denied Package Review package-review@lists.fedoraproject.org's request for Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com's needinfo: Bug 1816301: Review Request: openfoam - computational fluid dynamics https://bugzilla.redhat.com/show_bug.cgi?id=1816301
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
mark.olesen@esi-group.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(sanjay.ankur@gmai |needinfo- |l.com) |
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #22 from mark.olesen@esi-group.com --- Was prompted by the automatic review. I am still interested in submitting the package. In the meantime the spec file has been massively reworked to have a constant name, include all meta-packages, slots in properly with the newer Fedora mpi-lib structure etc.
/mark
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #23 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- That sounds good, Mark. Please submit the update spec/srpm whenever you are ready and we can continue the review.
Product: Fedora Version: rawhide Component: Package Review
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com has canceled Package Review package-review@lists.fedoraproject.org's request for Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com's needinfo: Bug 1816301: Review Request: openfoam - computational fluid dynamics https://bugzilla.redhat.com/show_bug.cgi?id=1816301
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(sanjay.ankur@gmai | |l.com) |
https://bugzilla.redhat.com/show_bug.cgi?id=1816301
--- Comment #25 from mark.olesen@esi-group.com --- Updated Spec URL: https://download.copr.fedorainfracloud.org/results/openfoam/openfoam/fedora-... Updated SRPM URL: https://download.copr.fedorainfracloud.org/results/openfoam/openfoam/fedora-...
package-review@lists.fedoraproject.org