@tgamblin : could you please have a look at the skeleton of the thing and tell me if the overall idea fits what you had in mind?

Looks likes a well-thought out idea so far. Can you please explain more about the use case and the problem(s) it solves? Thanks!

@citibeth One of the most requested features (and one that we need at our site) is the possibility to 'customize' the modulefiles that are written after a successfull installation. The customizations in which I am interested here will consist in the addition of new entries and/or in the modification of the usual entries that are added via automatic discovery of some standard directories. Also, these customizations may happen either per-package (most part of the cases) or per site.

As an example of a per-package customization you may think of an MPICH installation that could set MPICH_CC or similar variables in the modulefiles. IntelMPI would instead represent an example of a per-site customization, as the installation will be tuned to the right network fabrics, etc. setting a bunch of environment variables appropriately.

To reach the goal without creating a tight coupling among different parts of spack a viable approach would be to (imo) :

• group together the modifications that are done to the environment (and are now plain os.environ[name] assignment within some function) into an object
• build a mechanism that constructs one of these objects appropriately and produces the desired customizations (in another PR?)

@alalazo: ok I went over this. It looks really good! I like the design; I think this is definitely the right direction. High level, my comments are basically:

• A few suggestions to make things prettier.
• There are a few nits with setup_environment, setup_dependent_environment, and setup_extension_environment I'm not sure about. I think we probably need more than one method here b/c the env info goes in separate module files. I think the choice to factor out the module-scope modifications as you have done is a good one, too.
• We need to figure out how to get compiler wrapper variables like OMPI_CC to behave differently in and out of Spack. In Spack they should use Spack's compiler wrappers. Outside of Spack they should call real compilers.

I think that is it. Thanks!

@tgamblin I think I am done with the comments of your review. If you like the approach above I'll just clean the few TODO I left behind as a reminder and I will be ready to merge.

By the way, thanks for taking the time to review the code : I see that it's a one versus many battle and I imagine it's taking some effort!

@alalazo: I just pushed a branch with some additions to this that I think makes the interface a bit cleaner and avoids the need to patch anything. It's in epfl-scitas-features/env_objects_flying_around. See what you think.

Basically the interface looks like this now:

    def setup_environment(self, spack_env, run_env)
    def setup_dependent_environment(self, spack_env, run_env, dependent_spec)
    def setup_dependent_python_module(self, module, dependent_spec)

The intent is to make it clear when we're modifying the compile environment and when we're modifying the runtime/module environment. To make it clear, the two env methods have two environment module objects: spack_env and run_env. One modifies the compile environment and the other modifies the run environment, and changes to the run env will go into modules. It was possible to modify the compile env for a package before by simply setting env['foo'] = 'bar' within install(), but I think this factoring is nicer because it allows the package author to use the env modifications the same as all the other env settings if they like.

modify_module is renamed to setup_dependent_python_module to be consistent with the other methods, and to distinguish the python module scope from environment modules in the name.

I also tried to rename set_env and unset_env to set and unset in EnvironmentModifications (because the _env was repetitive most places it was used) and added set_path to simplify dealing with paths (user doesn't have to call concatenate_paths).

Anyway, see what you think. I think separating this a little more makes the code more clear and avoids the need to patch.

PS: I wish Github let us both iterate on the same PR. Do you know if that is possible? I do not see how I could commit to the PR branch. Bitbucket is kind of nicer in this regard b/c it has better branch permissions and you can interact with collaborators on a branch within the main repo.

@tgamblin I really like passing two different environments, in particular as it permits to avoid monkey patching the code (a behavior which would not have been obvious from user perspective). I also think that all the modifications to EnvironmentModifications effectively make the API more usable.

There are a few thoughts / remarks that I would like to share :

1. is it still necessary to have setup_environment at all? Before the idea was that the run-time environment was responsibility of setup_environment, while the spack environment was on setup_dependent_environment. Now that two env instances are passed to setup_dependent_environment is the other function still needed?
2. I would rename setup_dependent_python_module to setup_dependent_module, just to avoid people confusing the python module you are referring to with the python package provided by spack
3. there are a few missing things in openmpi and mpich (basically setting run_env) + mpich call seem to have a wrong signature

If you agree with point 1. in particular I can pull your modifications here and try to simplify the PR a bit further implement the use case below.

@tgamblin It seems that also edits to older messages are not notified to me. I just saw accidentaly your PS above (and that's kind of a shame, as I edit constantly my posts). If it may work for you, I've issued an invitation to our organization so to let you being able to push on PR branches.

@tgamblin On a second thought having both calls may be indeed useful. Here's the use case that comes to my mind : hdf5+mpi. I think its tcl module file should have depends_on <concrete-mpi> and it's the actual mpi that should add that to the run_env in setup_dependent_environment. If you agree I'll try to implement this case here.

@alalazo:

1. yes I think we both agree now that we need both. One allow you to set up your package's environment, the other allows you to set up something in the environment of dependencies. The Python case actually makes use of this -- extensions' modules need to add their own PYTHONPATH to the environment, but it depends on a) the version of Python (the extendee) and b) the prefix of the extension.
2. I'm not opposed to setup_dependent_module. I do worry that people will be confused by this, though, as they'll thing module refers to a TCL/Lmod module. What if we called it setup_dependent_package? Package at least is not quite as overloaded and you're really setting variables to be used by the dependent package.
3. I was going to comment on the OMPI and MPICH stuff. I took out the run_env settings there because the compilers are patched by filter_compilers. You don't actually need to have a module set OMPI_CC or MPICC_CC. Do you think the env vars should be set anyway?

I think its tcl module file should have depends_on and it's the actual mpi that should add that to the run_env in setup_dependent_environment.

I think this is useful, although the ORNL folks (@robertdfrench, others) mentioned that they have some more generic prereq logic that they would like to see added. Do you think this capability could be added later if this PR is mostly ready to go? I know @jgalarowicz would appreciate the capabilities that are already in here.

Aslo, did you see #498? Is this a decent naming scheme? It would be nice to support @glennpj's use case here, and I think his module names are better than the current builtin ones.

I think this is useful, although the ORNL folks (@robertdfrench, others) mentioned that they have some more generic prereq logic that they would like to see added. Do you think this capability could be added later if this PR is mostly ready to go?

Yes. Next on my list for module files I have :

• support for LMOD (i.e. update lmod : support for hierarchical modules #107 so that it will be ready to be merged)
• support for custom naming scheme (I'll take a close look at Modules in directories and fixes for setup-env.csh #498 tomorrow, maybe iterate with @glennpj on this)
• support for site-customization (i.e. add an hook to read variables from a configuration file)

and having this PR merged would surely help to proceed.

What if we called it setup_dependent_package

Ok.

@alalazo: awesome! So is this one ready to be merged?

@tgamblin Yes, the only detail I am wondering is if in modules.py we should iterate only on extendees (I originally meant it for python and the likes) or on all the dependencies. Maybe the latter is more appropriate right now. Besides that I think we can merge.

@alalazo: I think the Python package should probably iterate only on extendees, but I don't think we should add another callback function for that. Packagers can call dependent_spec.package.is_extension to check for this if they only want to affect extendees.

@tgamblin Ok then. Merge! 😄

Thanks for all the work on modules!

@robertdfrench @jgalarowicz: you should be able to add some variables to the generated module files now.

TODO: docs.

Oops -- forgot to merge my changes here. Sigh. I'll put those in.

@tgamblin Eheh, I thought you decided to merge this and then your changes from llnl branch...

Fixed. ☺️