Use Prefix class for build_directory for CMake and a few other builders.

[knoepfel]

The spack.util.prefix.Prefix class enables simple joining of path names. Users are accustomed to using this facility for the self.prefix attribute in a recipe's (e.g.) setup_run_environment(self, env) method. Something similar should exist for self.build_directory, but it does not appear to be uniformly followed.

This PR fixes that inconsistency.

[alalazo]

I would keep them as simple strings. Also, if I were to change the type I would probably try to use pathlib.Path more rather than our own substitute for that (developed when we had to support Python 2.6).

[knoepfel]

@alalazo, ... well ... I think simple strings are insufficient (and inconvenient!) as there's already precedent for something more powerful (like self.prefix in many of the package recipes). I am not be opposed to moving to pathlib.Path uniformly. But there are a lot of recipes out there that rely on the Prefix class, and it's prevalent in documentation (just look at all the uses of self.prefix... at https://spack.readthedocs.io/en/latest/packaging_guide.html).

One could redefine Prefix in terms of pathlib.Path as a means of migrating from the spack substitute to the Python-sanctioned one. I'd be willing to look into that, but in any case I'd like to have something that can be used predictably for both self.build_directory and self.prefix...and we don't have that.

Further thoughts? @becker33, I'm also interested in your take on this.

[alalazo]

@knoepfel I'm not saying I would change prefix in the API, I'm just saying I would not use it in other attributes.

[knoepfel]

@alalazo, maybe I'm not understanding you, but are you saying you're okay with the API inconsistency between self.build_directory and self.prefix?

As a user, I am not okay with that--it means I have to think harder about when to use one notation vs. another in the recipes even though both attributes represent a path. And as a Spack maintainer, I am not okay with the inconsistency and would like to fix it. If that means looking for a migration path from Prefix to pathlib.Path, then I'm happy to pursue it!

[alalazo]

I'm not understanding you, but are you saying you're okay with the API inconsistency between self.build_directory and self.prefix?

At the moment yes. Also note:

1. You're not making it consistent. There are many more path like attributes that are strings in build systems classes
2. It's an API break, particularly bad for custom repositories

[knoepfel]

I'm not understanding you, but are you saying you're okay with the API inconsistency between self.build_directory and self.prefix?

At the moment yes. Also note:

1. You're not making it consistent. There are many more path like attributes that are strings in build systems classes

I acknowledge that I'm not "fixing" the entire repository (that would take a lot of work!), but this PR makes it no less consistent, and it adds some usability improvements for those who already rely on similar features elsewhere.

2. It's an API break, particularly bad for custom repositories

Yes, it's an API break, but the chance of actual breakage is incredibly low as Prefix inherits from str.

[knoepfel]

@becker33, @alecbcs, or @tgamblin: I'd like to get some direction on what to do with this PR. I can think of at least three options:

1. Discard this PR and just have the build_directory attribute be a str as it is now.
2. Retain this PR and make the build_directory be an instance of the Spack-defined type Prefix to facilitate easier directory specifications (like self.prefix)
3. Adjust this PR so that the Spack-defined type Prefix inherits from pathlib.Path, thus providing a migration path for eventually removing Prefix in favor of pathlib.Path.

I think @alalazo's preference is either 1, or possibly something akin to 3. I am willing to do 3, but for now, I think 2 is already a nice improvement for recipe authors. Let me know.

[alecbcs]

I think either options 2 or 3 are a nice improvement. However we're about to cut the release for ISC and this could introduce bugs late in the release process due to the API change, my vote would be to hold off for a few weeks until after ISC and then either merge this as option 2 or 3. At which point we'll have until SC to fix possible bugs.

[alalazo]

My take:

However we're about to cut the release for ISC and this could introduce bugs late in the release process due to the API change, my vote would be to hold off for a few weeks until after ISC

:+1:

Yes, it's an API break, but the chance of actual breakage is incredibly low as Prefix inherits from str.

This is a common idiom in packages:

https://github.com/spack/spack/blob/d47951a1e3db6bbf884bf94a1abde2e0d5e1aa13/var/spack/repos/builtin/packages/adios/package.py#L86

The build directory is not something we inject and control, but something that packager can - and do - override. Some numbers:

$ grep "build_directory =" -R var/spack/repos/builtin | wc -l
179

[knoepfel]

However we're about to cut the release for ISC and this could introduce bugs late in the release process due to the API change, my vote would be to hold off for a few weeks until after ISC

Agreed, @alecbcs.

This is a common idiom in packages:

https://github.com/spack/spack/blob/d47951a1e3db6bbf884bf94a1abde2e0d5e1aa13/var/spack/repos/builtin/packages/adios/package.py#L86

The build directory is not something we inject and control, but something that packager can - and do - override. Some numbers:

$ grep "build_directory =" -R var/spack/repos/builtin | wc -l
179

Thanks for this, @alalazo -- this was a nuance I didn't appreciate before. I'd like to look things over again and re-evaluate what parts of this PR make sense. Stay tuned.