add package for swak4foam (SWiss Army Knife for Foam) #3650

olesenm · 2017-03-31T16:11:14Z

builds against all 'openfoam' providers

adamjstewart · 2017-03-31T16:36:03Z

var/spack/repos/builtin/packages/swak4foam/package.py

+# This file is not part of OpenFOAM, nor does it constitute a component of an
+# OpenFOAM distribution.
+#
+##############################################################################


You should probably leave this copyright alone. LLNL has some requirement that every python file in Spack has the same license.

We can discuss the background by email, when you have some time.

If @tgamblin approves then I'm okay with it.

adamjstewart · 2017-03-31T16:38:54Z

var/spack/repos/builtin/packages/swak4foam/package.py

+                return join_path(foam_spec.prefix, d)
+
+    raise RuntimeError('No OpenFOAM project directory found')
+    return None


It might be easier to set this in the openfoam providers. This would look like:

def setup_dependent_environment(self, spack_env, run_env, dependent_spec): spack_env.set('WM_PROJECT_DIR', join_path(self.prefix, 'etc', 'bashrc'))

You can do this for as many variables as you want.

Also, this return statement is redundant.

If I set this in the provider, where do these land? If the package is physically installed, I guess that they get buried in the .spack directory. But how would something like swak4foam get at these values? (I'm just starting with spack).

Q: is there a spack variable for dirname(self.prefix)? I'm using this in a few places and there must be something better.

If you set this in the provider, that environment variable is set in Spack's environment whenever you build things that depend on it. You can also set run_env if you'd rather have it end up in the module file.

What do you mean by dirname(self.prefix)? The directory above the installation prefix? Why would you want that? If you want the installation directory of a dependency, use self.spec['openfoam'].prefix.

adamjstewart · 2017-03-31T16:40:26Z

var/spack/repos/builtin/packages/swak4foam/package.py

+
+    variant('python', default=True, description='Build python modules')
+
+    depends_on('openfoam+sources')


source, not sources.

nice catch!

adamjstewart · 2017-03-31T16:42:10Z

var/spack/repos/builtin/packages/swak4foam/package.py

+# Force python to be found first
+PATH=$python_bin:$PATH
+#
+""".format(python_bin=spec['python'].prefix.bin))


This will work, some of the time. See #3367. I'm fine with this going in as is, but after #3367 is merged we'll want to change this.

It certainly looks and feels like a hack, just like the things with bison...

Nah, bison is fine. Python is commonly added as an external package. The problem with Python is that even if Python is installed in /usr/local/bin, the prefix may not be /usr/local. For Homebrew and macOS Python installations, the binary is symlinked to /usr/local/bin but the installation prefix is completely different. We could ask users to set the "real" prefix, but no one will know where to look or want to do that. Sometimes things need to "just work", which is what #3367 is for. Bison doesn't have this problem because it should always be located in spec['bison'].prefix.bin.

adamjstewart · 2017-03-31T16:43:16Z

var/spack/repos/builtin/packages/swak4foam/package.py

+
+        # Record the spack spec information
+        with open("log.spack-spec", 'w') as outfile:
+            outfile.write(spec.tree())


Do you actually need this information for anything? This is already recorded in <prefix>/.spack/spec.yaml.

adamjstewart · 2017-03-31T16:44:02Z

var/spack/repos/builtin/packages/swak4foam/package.py

+            outfile.write(spec.tree())
+
+        # Emit openfoam version
+        tty.info('Build for {0}'.format(spec['openfoam'].format('$_$@$%@+$+')))


Isn't this already printed when you install the package? No other package prints this line.

Added in a comment - basically want to know immediately which openfoam version is being pulled or will be built without waiting on it.

adamjstewart · 2017-03-31T16:44:58Z

var/spack/repos/builtin/packages/swak4foam/package.py

+
+    def install(self, spec, prefix):
+        """Install under the projectdir (== prefix)"""
+        self.build(spec, prefix)  # Should be a separate phase


You can make it a separate phase by adding:

phases = ['build', 'install']

above. Then this line will no longer be needed.

With the openfoam packages I tried the same (its in the comments).
If I ran spack install, I could see that it was running through all the phases properly. However, if I tried spack build, it complained about it not having a build phase separate from the install phase.
Now that openfoam-com has been merged, may be you could see if you encounter the same problem.

Yep, I noticed the same problem. See #3642.

adamjstewart · 2017-03-31T16:45:43Z

var/spack/repos/builtin/packages/swak4foam/package.py

+        args = []
+        if self.parallel:  # Build in parallel? - pass via the environment
+            os.environ['WM_NCOMPPROCS'] = str(self.make_jobs) \
+                if self.make_jobs else str(multiprocessing.cpu_count())


Use str(make_jobs). Otherwise, users won't be able to change the number of jobs they want with spack install -j <num>.

I know you mentioned this before, but if I stringify self.make_jobs it can be either "None" or "". Both are really bad for WM_NCOMPROCS, which should either be unset (make in parallel) or have an integer number for a parallel build. There is no convenient 'everything' setting.
The code snippets:

if [ -n "$WM_NCOMPPROCS" ] ... if [ "$WM_NCOMPPROCS" -gt 1 -a -z "$MAKEFLAGS" ] ... parOpt="-j $WM_NCOMPPROCS" ...

However, if I do request spack install -j 3 swak4foam, it seems to be doing exactly the right thing and only using 3 procs.

I don't think self.make_jobs can be none if parallel == True, but I could be wrong.

Don't use self.make_jobs. Just use make_jobs. It's in global scope and set up in set_module_variables_for_package in build_environment. It's always a number (e.g., 1 if not parallel). I think we could better document this, or have a namespace where all this stuff comes in, instead of the module magic.

adamjstewart · 2017-03-31T16:46:10Z

var/spack/repos/builtin/packages/swak4foam/package.py

+            "spack-build.out", "log.spack-spec",
+            self.config_file,
+            'COPYING',
+        ]:


Already in <prefix>/.spack/.

adamjstewart · 2017-03-31T18:22:01Z

var/spack/repos/builtin/packages/swak4foam/package.py

+        branch='branches/develop',
+        git='https://github.com/Unofficial-Extend-Project-Mirror/openfoam-extend-swak4Foam-dev.git') # noqa
+    # The git mirror can be more convenient than using hg
+    # http://hg.code.sf.net/p/openfoam-extend/swak4Foam


What is more convenient about it? Spack also has Mercurial support that works similarly to our git fetching.

only thing I can think is that Spack's git support does shallow clones and is pretty intensely optimized, while Spack's hg is not...

Yes, shallow clone and with git 'branch' I have a fair better idea of what I am getting. With hg, I'm also totally illiterate and can't make any upstream adjustments without creating complete chaos.

added comment in code

adamjstewart · 2017-03-31T18:26:45Z

var/spack/repos/builtin/packages/swak4foam/package.py

+# Force python to be found first
+PATH=$python_bin:$PATH
+#
+""".format(python_bin=spec['python'].prefix.bin))


Nah, bison is fine. Python is commonly added as an external package. The problem with Python is that even if Python is installed in /usr/local/bin, the prefix may not be /usr/local. For Homebrew and macOS Python installations, the binary is symlinked to /usr/local/bin but the installation prefix is completely different. We could ask users to set the "real" prefix, but no one will know where to look or want to do that. Sometimes things need to "just work", which is what #3367 is for. Bison doesn't have this problem because it should always be located in spec['bison'].prefix.bin.

adamjstewart · 2017-03-31T18:28:14Z

var/spack/repos/builtin/packages/swak4foam/package.py

+        # Emit openfoam version prior to build
+        # We may be resolved the wrong version, and don't wish to build
+        # it as a dependency
+        tty.info('Build for {0}'.format(spec['openfoam'].format('$_$@$%@+$+')))


I still don't think this is the right thing to do. This could be said about every package in Spack. Spack already tells you what it is going to install before it installs it. If you think we could do a better job of this, we should do it for every package, not just this one.

I guess this depends on the size of the underlying package. If I accidentally build the incorrect OpenFOAM version/variant (eg, wrong float size, etc), I've wasted 15-20 min on 24 cpus. Emitting a single line seems to be a small overhead to avoid these types of mistakes.

tgamblin · 2017-04-01T00:38:46Z

var/spack/repos/builtin/packages/swak4foam/package.py

+    version(
+        'dev',
+        branch='branches/develop',
+        git='https://github.com/Unofficial-Extend-Project-Mirror/openfoam-extend-swak4Foam-dev.git') # noqa


this doesn't need the noqa -- we exempt URLs.

olesenm · 2017-04-06T08:31:34Z

updated to work with changed openfoam environment

adamjstewart

One last comment, otherwise looks good.

adamjstewart · 2017-06-21T16:41:47Z

var/spack/repos/builtin/packages/of-swak4foam/package.py

+import os
+
+
+class OfSwak4foam(Package):


I am in favor of renaming this package to be swak4foam instead of of-swak4foam. When someone searches for it in Spack, I think they are more likely to look for swak4foam.

Ok, I could go either way. I figured that the of- prefix would make for nice sorting and collation of openfoam-related work, but going for the original name also makes sense.

I'll rejig in the next days (maybe next week) and ping you when it's ready.

adamjstewart · 2018-03-21T03:13:24Z

@olesenm Sorry I haven't looked at this PR in a while. I'm on spring break this week so if you have time I would love to get this package in.

samcom12 · 2022-01-14T09:02:19Z

Any update on this?

olesenm · 2022-01-14T15:26:44Z

Any update on this?

I was surprised to see that there might actually be any further interest in this. Feel free to pick up and add updated swak versions. Optionally, if there are particular swak-type of functionality needed in OpenFOAM, I would tend to place more effort there. In the years since this original changeset, openfoam has added a number of expression handling very similar to swak, but based on ragel (for lexing) and lemon (for the parser).

adamjstewart requested changes Mar 31, 2017

View reviewed changes

olesenm force-pushed the swak4foam branch 2 times, most recently from 260fd30 to bf4139c Compare March 31, 2017 18:05

adamjstewart requested changes Mar 31, 2017

View reviewed changes

tgamblin reviewed Apr 1, 2017

View reviewed changes

olesenm force-pushed the swak4foam branch 3 times, most recently from 9948fda to 2441049 Compare April 6, 2017 08:27

olesenm force-pushed the swak4foam branch from 2441049 to 6e833de Compare April 6, 2017 23:42

adamjstewart added the new-package label Apr 9, 2017

adamjstewart mentioned this pull request Jun 21, 2017

refactor openfoam packages #3669

Merged

adamjstewart reviewed Jun 21, 2017

View reviewed changes

new package swak4foam (SWiss Army Knife for openFoam)

bdc681e

olesenm force-pushed the swak4foam branch from 6e833de to bdc681e Compare July 4, 2017 13:01

alalazo requested a review from nrichart March 23, 2018 08:06

olesenm closed this Jun 28, 2018

olesenm deleted the swak4foam branch June 28, 2018 08:01

olesenm restored the swak4foam branch June 28, 2018 08:01


		variant('python', default=True, description='Build python modules')

		depends_on('openfoam+sources')

		import os


		class OfSwak4foam(Package):

add package for swak4foam (SWiss Army Knife for Foam) #3650

add package for swak4foam (SWiss Army Knife for Foam) #3650

Conversation

olesenm commented Mar 31, 2017

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

olesenm Mar 31, 2017 • edited

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

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

Choose a reason for hiding this comment

tgamblin Apr 1, 2017 • edited

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

olesenm Apr 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olesenm commented Apr 6, 2017

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

adamjstewart commented Mar 21, 2018

samcom12 commented Jan 14, 2022

olesenm commented Jan 14, 2022 • edited

olesenm Mar 31, 2017 •

edited

tgamblin Apr 1, 2017 •

edited

olesenm Apr 6, 2017 •

edited

olesenm commented Jan 14, 2022 •

edited