refactor openfoam packages #3669

olesenm · 2017-04-03T15:25:20Z

use common OpenfoamArch class to manage common openfoam naming and compiler handling.
Only carp about unsupported arch/compiler at the build stage, which should improve robustness.
export special-purpose FOAM_PROJECT_DIR env for dependent packages, which can be used to locate the etc/bashrc file.

fixup handling of make_jobs (now use global).

olesenm · 2017-04-05T12:29:56Z

I think that the packages now look reasonably good. However, I have only exported an absolute minimal modules environment. This would need more attention, and the knowledge of someone with more experience there.

adamjstewart · 2017-04-07T18:27:11Z

@olesenm I promise I'm not ignoring your OpenFOAM PRs. There's just a lot to review here and I'm not very familiar with OpenFOAM. Most Spack packages are 5-10 lines long. I assume there's no way to convince the various OpenFOAM developers to switch to CMake or Autotools? 😆

One thing worth doing would be to include all of the variables set in $FOAM_PROJECT_DIR/etc/bashrc in the resulting module file. Right now, if you want to use OpenFOAM, you have to run:

$ module load openfoam-org
$ source "$FOAM_PROJECT_DIR/etc/bashrc"

If you add the appropriate environment variables to the module file, you won't need to run the second command anymore. See intel-parallel-studio for a similarly complex example of how to set environment variables that get added to the generated module file.

olesenm · 2017-04-10T19:06:15Z

I have the same concern about environment variables, but I want to leverage the normal OpenFOAM bashrc information and not hardcode the equivalent logic inside of the package. Otherwise I greatly fear that the environment will very quickly become out of sync with what the bashrc logic produces.
Unfortunately using EnvironmentModifications.from_sourcing_files() bombs out. Besides which, it isn't flexible enough. We'd need to source the bashrc (for/from stage.source_path), replace the prefix/project directories with their final locations. Purge a few entries and/or replace them with more generalized values (partly using the user's environment). Example,

setenv FOAM_USER_APPBIN  "$env(HOME)/$env(USER)-$WM_PROJECT_VERSION/..."

This unfortunately also needs to go back into the LD_LIBRARY_PATH and it doesn't look like the current spack tools will be useable here. I have an alternative (hack) to do the same thing and convert the environment changes to a ModuleInclude.tcl, but no obvious way to integrate this into the spack install, nor if this is even a sane approach in the first place.

adamjstewart · 2017-04-10T19:11:05Z

Unfortunately using EnvironmentModifications.from_sourcing_files() bombs out.

Yeah, I've seen the same problem. The problem is that setup_environment runs before the installation, so there's no way to add it to your module file post-installation. @alalazo I wonder if we should run setup_environment pre-installation to set spack_env and then run setup_environment again post-installation to add run_env to the module file. Or create two separate functions. The same problem affects the intel-parallel-studio package, which also comes with a handy bashrc that we have manually added to Spack. As development of these projects continues, these environment variables will change and become trickier to manage.

nrichart

I tried the foam-extend package it works fine. But if the aim is to refactor the code I think there is still room to have more code in common, for example the properties or the functions like setup_environment are copy pasted and the spack-Allwake or readme files. Would it be feasible to wirte a Package class and inherit from it.
It would make sense to have a build_systems/wmake.py

nrichart · 2017-04-11T13:20:41Z

var/spack/repos/builtin/packages/foam-extend/package.py

-        return 'foam-extend-{0}'.format(self.version.up_to(2))
+        for d in ['wmake', self.archbin]:  # bin already added automatically
+            run_env.prepend_path('PATH', join_path(self.projectdir, d))
+        run_env.set('MPI_BUFFER_SIZE', "20000000")


Is this a requirement for all MPIs ?

This is always set by the OpenFOAM environment scripts and seems to apply to all MPIs except SGI. Presumably this could be addressed if/when the setup_environment gets sorted.

nrichart · 2017-04-11T13:46:45Z

var/spack/repos/builtin/packages/foam-extend/assets/spack-Allwmake

+)
+fi
+
+# -----------------------------------------------------------------------------


This file could be factorized with the one in openfoam-com
This 2 files are to avoid using from_sourcing_files ?

The files are almost identical, except that the handling of FOAM_INST_DIR has changed somewhat. The problem with from_sourcing_files seems to be unrelated to the installation directory. Maybe it has issues with some of the shell functions? I don't really know.

The from_sourcing_files appears to bomb out (ImportError can't find site) when using my system python (2.7.12), but seems to work if I make python a build requirement for openfoam.

Nonetheless, I still can't do much with the environment settings since they called before the files are actually installed.

How it was done before was by creating a EnvironmentModifications en applying it to the current environment, this can be done in the install function.

def get_openfoam_environment(self): return EnvironmentModifications.from_sourcing_files( join_path(self.stage.source_path, 'etc/bashrc'))

That can be used anywhere like

env_openfoam = self.get_openfoam_environment() env_openfoam.apply_modifications()

But again by writing a WMakePackage this might be solved if the order of the stages can be a bit rearranged. I have to ask @alalazo if it is doable. In this case What you put in common in OpenfoamArch could be inherited and make it easier to factorize code.

If it makes sense, writing a WMakePackage is absolutely doable. All you need to do is have a new module under the build_system folder and collect in the base class whatever can be reused. All the OpenFOAM forks (3 by now?) will inherit from WMakePackage and will contain only the part that are different from one to another.

nrichart

All the discussed possible change are a bit stuck do to changes that would be needed in the core of spack. So I would approve this PR for now. And hope some improvements could be done with some new features of spack in a near future like #3183.

-- Refactor openfoam packages by adding an OpenfoamArch class Use separate configure, build, install phases. Provide FOAM_PROJECT_DIR dependent env for openfoam packages - easier way to locate Eliminate intermediate installation directories - unneeded clutter. - makes it less than easy to find the etc/bashrc file Add versioning for all openfoam patches - no certainty which parts (if any) will be needed in future versions, especially if we strive to ensure that the upstream version builds well with spack to begin with. Support build of develop branches - helps track build regressions for future openfoam releases STYLE: use common/ and assets/ to provide additional (build) resources ...

Move openfoam-com up front since this is the one being used as a base for the others

adamjstewart

Just added a couple last minute comments.

Are you an OpenFOAM developer? It would be nice if OpenFOAM converted to a standard build system like CMake, but I suppose it's too late for that now. It's a shame that OpenFOAM relies on so many environment variables to build and run. For most packages in Spack, if you add depends_on and a couple flags, that's all you need to install large software stacks. Thanks to RPATH, as long as the prefix.bin directory is in your PATH, everything should "Just Work(TM)", but this obviously isn't the case with OpenFOAM.

Sorry that it's taken so long to review this PR. Most packages are under 20 lines, so when I see a 1,000+ line PR that only affects a few packages, I tend to put it off. I'm inclined to just merge this as it doesn't seem like OpenFOAM will magically become "easy to install" any time soon. Same for your OpenFOAM extensions, which I'll review after I merge this PR.

adamjstewart · 2017-06-15T14:51:42Z

var/spack/repos/builtin/packages/foam-extend/package.py

@@ -111,189 +107,77 @@ class FoamExtend(Package):
    depends_on('scotch~metis+mpi', when='+ptscotch')
    depends_on('metis@5:',         when='+metis')
    depends_on('parmetis',         when='+parmetis')
-    depends_on('parmgridgen',      when='+parmgridgen')
+    # mgridgen is statically linked
+    depends_on('parmgridgen',      when='+parmgridgen', type='build')


Might want to keep this as the default: build/link. Otherwise Spack's compiler wrappers won't automatically add -L/path/to/mgridgen/lib.

I understand, but it will get linked in via the OpenFOAM build logic. In this case spack is reduced to feeding the OpenFOAM config with appropriate paths of where to find things and the OpenFOAM build manages the rest.

Is there any reason not to use the default dependency type? Just because OpenFOAM is special doesn't mean it doesn't link to that library.

adamjstewart · 2017-06-15T14:54:04Z

var/spack/repos/builtin/packages/foam-extend/package.py

+        variable since it would mask the normal OpenFOAM cleanup of
+        previous versions.
+        """
+        spack_env.set('FOAM_PROJECT_DIR', self.projectdir)

    @property
    def projectdir(self):


Is this attribute useful for anything? Why not always use prefix? That's what every other package in Spack does.

Hi Adam,

For the top bit.
Yes I am an OpenFOAM developer (ESI-OpenCFD: www.openfoam.com).
Moving to another make system is fairly unlikely at this point. The hand-rolled wmakesystem is actually just a bunch of wrappers around a standard make, but obviously without all of the autotools or cmake configuration guessing. The main distinction though is probably that we have various lnInclude (symlinked include directory) with the headers for each major subsection. This means that the make file has the directory reference (via the -I... compiler option) without needing really long #include ... lines everywhere. If we go away from that, everything breaks and we have thousands of files...

OpenFOAM is indeed still a monster for using environment variables. The normal spack prefix is mostly okay, but I need to be able to build OpenFOAM with spack, but also be able to source the OpenFOAM environ and build depend modules without necessarily using spack. For some (older) versions of OpenFOAM we some weird logic within OpenFOAM to find where the bashrc/cshrc files are located. I hope that we can make this even smoother in the future and drop this variable too.

Once the modules support fits better we should also be able to improve. If we can get this pull request merged in, any subsequent adjustments will presumably return the 'normal' range of 10-20 lines.

Cheers,
/mark

@olesenm Makes sense. I can only imagine how difficult it would be to make such a substantial change to the build system, especially now that OpenFOAM development has been split into 3 different branches: openfoam-com, openfoam-org, and foam-extend. I assume you primarily develop openfoam-com?

Also, thank you so much for taking the time to get these packages to build. I remember the last time one of our users requested OpenFOAM (org), I spent several weeks just trying to get it to build. I spent another several weeks trying to package it into Spack based on the foam-extend package, but had no luck. It's really nice when developers package their own software because I imagine you know what you're doing a lot better than I do.

adamjstewart · 2017-06-21T16:35:25Z

I'm just going to merge this. There is certainly an enormous amount of room for future improvement and ways that we can cut back on code that is duplicated across all 3 packages, but I don't want that to hold up @olesenm's work. It's a rare opportunity to see a developer interested in packaging their own software, and I don't want to scare off @olesenm or anyone else from contributing to Spack. God knows I would not want to have to write these packages myself.

Thanks @olesenm for all of your contributions! I'll take a look at #3650 and #3726 next.

* Several improvements for the openfoam packages -- Refactor openfoam packages by adding an OpenfoamArch class Use separate configure, build, install phases. Provide FOAM_PROJECT_DIR dependent env for openfoam packages - easier way to locate Eliminate intermediate installation directories - unneeded clutter. - makes it less than easy to find the etc/bashrc file Add versioning for all openfoam patches - no certainty which parts (if any) will be needed in future versions, especially if we strive to ensure that the upstream version builds well with spack to begin with. Support build of develop branches - helps track build regressions for future openfoam releases STYLE: use common/ and assets/ to provide additional (build) resources ... * - adjust OpenFOAM provider Move openfoam-com up front since this is the one being used as a base for the others

olesenm force-pushed the openfoam branch 2 times, most recently from bca3bd4 to 7eb39e5 Compare April 3, 2017 18:25

davydden requested a review from nrichart April 3, 2017 18:53

olesenm force-pushed the openfoam branch 4 times, most recently from 7552779 to aa51359 Compare April 5, 2017 12:27

nrichart reviewed Apr 11, 2017

View reviewed changes

olesenm force-pushed the openfoam branch from aa51359 to 3773001 Compare April 19, 2017 13:46

adamjstewart added the refactoring label Apr 28, 2017

olesenm force-pushed the openfoam branch from 3773001 to 24bbdad Compare May 15, 2017 09:27

nrichart approved these changes May 15, 2017

View reviewed changes

mark added 2 commits May 24, 2017 13:57

- adjust OpenFOAM provider

839e082

Move openfoam-com up front since this is the one being used as a base for the others

olesenm force-pushed the openfoam branch from 24bbdad to 839e082 Compare May 24, 2017 12:45

adamjstewart reviewed Jun 15, 2017

View reviewed changes

adamjstewart approved these changes Jun 21, 2017

View reviewed changes

adamjstewart merged commit 1f2e56e into spack:develop Jun 21, 2017

olesenm deleted the openfoam branch June 30, 2017 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor openfoam packages #3669

refactor openfoam packages #3669

olesenm commented Apr 3, 2017

olesenm commented Apr 5, 2017

adamjstewart commented Apr 7, 2017

olesenm commented Apr 10, 2017

adamjstewart commented Apr 10, 2017

nrichart left a comment

nrichart Apr 11, 2017

olesenm Apr 18, 2017

nrichart Apr 11, 2017

olesenm Apr 18, 2017

olesenm Apr 18, 2017

nrichart Apr 24, 2017

nrichart Apr 24, 2017

alalazo Apr 24, 2017

nrichart left a comment

adamjstewart left a comment

adamjstewart Jun 15, 2017

olesenm Jun 21, 2017

adamjstewart Jun 21, 2017

adamjstewart Jun 15, 2017

olesenm Jun 21, 2017

adamjstewart Jun 21, 2017

adamjstewart commented Jun 21, 2017

refactor openfoam packages #3669

refactor openfoam packages #3669

Conversation

olesenm commented Apr 3, 2017

olesenm commented Apr 5, 2017

adamjstewart commented Apr 7, 2017

olesenm commented Apr 10, 2017

adamjstewart commented Apr 10, 2017

nrichart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrichart left a comment

Choose a reason for hiding this comment

adamjstewart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamjstewart commented Jun 21, 2017