-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add config.yaml file #2152
Add config.yaml file #2152
Conversation
This looks awesome! Only thing, can we change: dirty: false to clean: true @citibeth has mentioned avoiding negative logic before and I think it makes sense in this case. |
Looks very good! Maybe docs could be added to your checklist. |
@adamjstewart: is I don't feel too strongly about this but |
@tgamblin In this case, "dirty" implies "not clean". It's a matter of whether or not we are cleaning up environment variables, not a matter of whether or not we want Spack to be "dirty". But I understand it matches the option flag. I guess the annoying thing is we don't have an option with the flag name. |
I could just was easily argue that |
Then why did you use "NOT clean" in the description of the flag? Because "dirty" doesn't actually mean anything 😆 |
I agree it's ambiguous here which one is negative logic. But I think the On Thu, Oct 27, 2016 at 4:13 PM, Adam J. Stewart notifications@github.com
|
Ok, everything but docs is done. Can people try this out? I went with |
From the tests :
|
Now that we have a |
@citibeth That's what this PR does. You can set |
Great, thanks! On Fri, Oct 28, 2016 at 11:06 AM, Adam J. Stewart notifications@github.com
|
Wouldn't it be useful to have an option for |
@pramodk: what do you mean? You want to be able to default the log format to something? I think the raw output is a good default, and the XML output is good for tests -- is there a reason you'd want the XML output all the time? |
I mean to enable |
That might be useful -- let's get this merged and then see if we need it. I was thinking that for testing we would likely just call the command with the |
3b4a74f
to
ad61950
Compare
Ok this is all done and should be ready to go. Bugs are fixed and docs are in. Can I get a review? @alalazo @adamjstewart @mamelara @mathstuf @justintoo @hegner |
Will make Spack take compilers *only* from the user configuration, and | ||
the site configuration will be ignored. | ||
See the documentation on :ref:`configurations scopes | ||
<configuration-scopes>`_ for details on how Spack's configuration system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trailing underscore is causing the documentation tests to fail. I think the trailing underscore is only needed for external URLs, not internal references. Gotta love .rst!
@adamjstewart Ok fixed up the docs. Added a bit more text to the page for |
Also, @hegner: I took out the ability to customize the My rationale is that I think if we allow install tree layouts to be customizable, it should be customizable through some configuration (e.g., |
# | ||
# Per-spack-instance settings (overrides defaults): | ||
# $SPACK_ROOT/etc/spack/config.yaml | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
etc/spack/config.yaml
should be added to .gitignore
. We don't want to normally check site configuration back into Spack. (If this is not added, config.yaml
will end up accidentally in some PRs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. /etc/spack/*.yaml
is already in .gitignore
# This is the path to the root of the Spack install tree. | ||
# You can use $spack here to refer to the root of the spack instance. | ||
install_tree: $spack/opt/spack | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making $spack
upper case, to align with common bash usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really hate uppercase. And this is YAML. Also it's been this way for a while... the default repo location uses this syntax in /etc/spack/defaults/repos.yaml
# Locations where different types of modules should be installed. | ||
module_roots: | ||
tcl: $spack/share/spack/modules | ||
lmod: $spack/share/spack/lmod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Spack package for this is called environment-modules
; I would suggest the same name be used here, rather than tcl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not, as:
- this is consistent with the format in
modules.yaml
tcl
identifies the type of module files written rather than the tools that understand them
The case for lmod
was discussed in #107 and lmod
was chosen over hierarchical_lua
because of established conventions in the HPC communities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for (1)... if it were changed, it would have to be changed everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I like the tcl
designator, mainly because everything under modules:
is technically an "environment module". But lmod
, tcl
, and dotkit
is how most people (at least at LLNL) refer to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should put parenthetically the first couple of times tcl
is mentioned, that it's really environment-modules
.
|
||
# Cache directory already downloaded source tarballs and archived | ||
# repositories. This can be purged with spack purge | ||
source_cache: $spack/var/spack/cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think spack cache purge
or spack purge cache
would be more descriptive than spack purge
|
||
# Cache directory for miscellaneous files, like the package index. | ||
misc_cache: ~/.spack/cache | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this also purged with spack purge
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I should put the right options to Spack purge on there. Yes, you can purge this with spack purge -u/--user-cache
. That's kind of inconsistent with the naming here, so I'd say we should change the argument to spack purge -m/--misc-cache
, since it could go someplace other than the user home directory. The distinction is really that the stuff in this cache is long-lived and small, so it doesn't need to be purged. The stuff in the build stage is short-lived, large, and purging doesn't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need better terminology, the word "cache" is inherently confusing (since there are two caches). How about spack purge --misc-cache
and spack purge download-cache
?
higher precedence scopes, as defaults may change from version to | ||
version. | ||
|
||
2. **site**: Stored in ``$(prefix)/etc/spack/``. Settings here affect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... the file is not copied from defaults to site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The files have never been copied...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be made clear in the docs...
|
||
3. **user**: Stored in the home directory, ``~/.spack/``. These settings | ||
affect all instances of Spack and take the highest precedence. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need a project scope. Typical use case... I'm building two software stacks with different version/dependency selections in them. Or I want to run an integration test with a packages.yaml
different from my "normal" one. Suggested ways to find the project scope would be:
- An env var
- A command line argument. (This would have to be available on all commands, ideally parsed and set up before the command is run. So then one could
alias myproject-spack 'spack --config ~/myproject/dotspack'
- Search up the source tree for a
dotspack
directory.
I would lean toward (2) as probably the best way to do this. As long as the Bash alias works, users can easily set up as many per-project Spack-like commands as they need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but this is a new feature not on the list for 0.10 and we should save it for another PR.
Commands that modify scopes (e.g., ``spack compilers``, ``spack repo``, | ||
etc.) take a ``--scope=<name>`` parameter that you can use to control | ||
which scope is modified. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do any commands automagically modify scope? For example, the first time you run Spack? In that case, what scope will be modified? Should Spack ask for confirmation before modifying it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler detection is run the first time you start Spack, and compilers.yaml
is put in the highest precedence scope available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that should ask confirmation because first-time users have no idea what that means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, no asking for confirmation. It still needs to be decided (and documented) which scope is modified when there is a command that automagically modifies configuration.
4. ``site/<platform>`` | ||
5. ``user`` | ||
6. ``user/<platform>`` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- How do we tell what
<platform>
is for our configuration? - Are any platform-specific files included in defaults?
With per-project configurations, user/<platform>
might be less necessary. But it's nice to have options...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make this clearer.
scopes, Spack will merge the settings between the two files. You can see | ||
the results of this merge with the ``spack config get <configtype>`` | ||
command. For example, if your configurations look like this: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this stuff has been there -- it just wasn't documented outside config.py
. I realized that it probably needed to go in the real docs, since the config system in Spack is actually halfway decent at taking options from many different places/roles and combining them.
I look forward to seeing this merged (if nothing else because it touches so many files). The biggest thing I think is messing is per-project scope. I really think that needs to go in before this is merged. |
8a1fbde
to
1e07e4a
Compare
Ok, @citibeth brought up some good points about the docs. I've reorganized them and tried to address all the concerns -- I think things are looking much better. New stuff:
Various refactors:
I think that is more than enough for one PR. FWIW, I'd like to get this merged soon so try to get any additional requests in soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only minor docs suggestions.
etc.) take a ``--scope=<name>`` parameter that you can use to control | ||
which scope is modified. By default they modify the highest-precedence | ||
scope. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...meaning, they modify the highest-precedence scope that exists? So if you don't have a user scope, it will modify the site scope? This should be made crystal clear. Also, I'm not sure it's the right choice; since clueless users can then accidentally modify the site scope. Probably best for config-modifying commands to just modify the user scope by default --- creating a new one if none exists.
|
||
When spack queries for configuration parameters, it searches in | ||
higher-precedence scopes first. So, settings in ab higher-precedence | ||
file can override those with the same key in a lower-precedence one. For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ab is typo?
file can override those with the same key in a lower-precedence one. For | ||
list-valued settings, Spack *prepends* higher-precedence settings to | ||
lower-precedence settings. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you delete things from lower-precedence scope? If deleting is not yet possible, that should probably be noted in the docs. But I think it's a feature people will want at some point.
I see below that the double-colon notation is mentioned. It should probably be at least referenced right here, so readers aren't left on the hook wondering if deleting is possible.
override lower-precedence settings. To do this, you can use *two* colons | ||
at the end of a key in a configuration file. For example, if the | ||
**site** ``config.yaml`` above looks like this: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, this is how things can be "deleted". I think this makes sense to me, although the double-colon notation is somewhat subtle (but maybe standard YAML?)
3d52c24
to
ee4991d
Compare
- Added a schema for config.yaml - Moved install tree configuration to config.yaml - Moved etc/spack/install.yaml to etc/spack/defaults/config.yaml - renamed install_area to "store", to use a term in common with guix/nix. - in `config.yaml` file, it's called the `install_tree` to be more intuitive to users. - `install_tree` might've worked in the code, but `install_tree` is already a global function in the spack namespace, from llnl.util.filesystem.
- Moved temp finding logic to spack.stage - Updated stage tests - Added tests for new path substaitution of $user, $spack, $tempdir
- Added new preferences to config.yaml: - source_cache - misc_cache - verify_ssl - checksum - dirty
- generalized and fixed to work with any key in YAML file - simplified schema writing, as well - add more unit tests for the config system - Rename test/yaml.py to test/spack_yaml.py - Add test/yaml.pyc to ignored pyc files.
Ok this is (finally) done. This PR actually resulted in a lot more changes than @hegner's original, but it's a bunch of changes that Spack really needed. The config system is improved and had some bugs fixed, and things are documented much more thoroughly. One thing that remains unfinished here is the default config scope behavior. Comment on #2184 if you care about that one -- I didn't feel like holding this up more for that. |
Rework of #908. Fixes #635, #1925.
This is a continuation of @hegner's #908, which allowed customizing the install location. This adds a top-level
config.yaml
with many commonly set properties.config.yaml
consolidates a lot of the things currently inspack/__init__.py
.Todo:
config.yaml
fileinstall_tree
option toconfig.yaml
build_stage
from__init__.py
source_cache
misc_cache
config.yaml
verify_ssl
from__init__.py
checksum
from__init__.py
dirty
from__init__.py
Below is what the
config.yaml
file looks like currently. Just need to wire in the features.Thoughts? @alalazo @adamjstewart @mamelara @mathstuf @justintoo @hegner