Ok, somebody help me out here.

It looks like codedev is complaining that I've touched a core spack
library (I have) that is not well covered by the unit tests.

How do I move forward? Do I have to write a unit test for the
sections of code that I've touched? Do I have to get a spack
developer to look at the patch and say it's ok?

Ping Todd @tgamblin who suggested that I submit the pull request.

--Mark

How do I move forward? Do I have to write a unit test for the sections of code that I've touched?

In general that is desirable but isn't strictly required. It may be difficult for options like the one added here.

Do I have to get a spack developer to look at the patch and say it's ok?

Yes. I think I'd have a chance to look at this by the end of Jan. 17 (which may include suggestions for testing but at first glance it looks tricky).

EDIT (1/17): I should be able to look at this by the end of 1/18

Thanks!

One way to test this is to write a small config.yaml file that sets a
few values, copy it into one of the configuration scope directories,
run 'spack config get config' and grep the output. For example,

$ spack config get config
config:
install_tree: $spack/opt/spack
...

$ spack -C my-config-dir config get config
config:
install_tree: /home/krentel/config/my-new-install-dir
...

That's what I did manually while writing the patch. What I don't know
is how to write this in spack's testing infrastructure or what related
tests there are now.

But first things first. My priorities are:

1. Get someone to sign off on the early option parsing. It's a
   legitimate objection, but without some major rework, I don't see
   another way.

2. Write documentation for how and why to use the option.

3. Look at writing a unit test. But this will take longer because I
   have to figure out the testing infrastructure and what existing tests
   there are now.

Get someone to sign off on the early option parsing.

I've read through this now and I'm not necessarily opposed to it but I want to try brainstorming some other approaches. First thought:

• Move all ConfigScope declarations in config.py into a method
• Don't set spack._config in __init__ (or any of the variables which depend on it): either set those globals from main or create a function in __init__ to set them (and call that from main)

The more I think on it, it does seem to be a fair bit of work, but I think it would help clean things up for other aspects of configuration that the user would want to affect from the command line. Perhaps this is not a worthy goal because for all such configuration either (a) it is contained in a scope (and therefore handled here) or (b) one can wait until main executes. I'd have to think on that more.

As you can tell, the fundamental problem is that spack reads the yaml
files before creating the argument parser. That makes it impossible
to add a command-line option that influences how/where it reads the
config files, at least not in a structured way.

The way I'd prefer to move forward is (1) add this patch now,
then (2) look at reworking the startup sequence.

In defense of the early scanning, it's small, does only one thing,
doesn't modify anything and it's very easy to rip out after reworking
the startup sequence.

I started looking at the use of _config a bit. I didn't see a
fundamental, fatal dependency (ie, cycle) that would make this
near impossible.

But my concern is that once you start pulling on this thread, there
may be more and more things that need to be reworked. What's the risk
it may take months? What's the risk of breaking something?
Is this really the best use of someone's time?

I'm entirely onboard if someone wants to do this, assuming they have
the time. But I have to beg off due to time, lack of knowledge and
fear of breaking something.

Thanks.

Peter,

Has the brainstorming turned up any more ideas? As I mentioned, I'd
prefer to move forward as is. That eliminates any time pressure on
reworking the startup sequence.

What's the risk it may take months? What's the risk of breaking something? Is this really the best use of someone's time?

The implied handling of an argument listed in main.py will create confusion for future work on argument handling for Spack. My current opinion is that if I merged this as-is, I'd want to know that I could schedule refactoring it personally within a month or so (since it sounds like you don't have the time for it). I can't do that at this point.

That being said if I were to think on it more and decided a refactor would not be necessary then I'd be more inclined to merge. That follows thoughts like

Perhaps [refactoring] is not a worthy goal because for all such configuration either (a) it is contained in a scope (and therefore handled here) or (b) one can wait until main executes. I'd have to think on that more.

So if you have thoughts along those lines I would be interested to hear them. I anticipate having more time to think on this myself around 1/31

There is one other possibility, short of reworking the startup
sequence.

We could require that the -C/--config option, if specified, must
always come first. That makes the early option scanning completely
trivial, just check if sys.argv[1] == '-C', else do nothing.

It also makes the early parsing oblivious to any other options.
Doesn't need to know what the other options are, or how many arguments
they have, etc. You could add another parser.add_argument() in
main.py without having to coordinate with init.py.

Would that be a satisfactory interim solution?

Short of that, it looks impossible without reworking at least
something in the startup sequence.

And I do think a command line option is useful. Without that, the
only way to maintain multiple configurations is to manually swap
files, or use multiple spack repos (what I do now).

I'd like to revisit this issue and find some way to move forward, or
else give up.

I suggest that we require that the -C option, if specified, must
always come first. That means that the "special" early option parsing
is completely trivial and totally isolated from the main options. You
just test if sys.argv[1] == '-C', else do nothing.

Is that a satisfactory solution?

If yes, then I'll rewrite the patch to only look for -C as the first
option. It's a really small patch and isolated from the main option
parsing.

ping @tgamblin

I actually have a rework forthcoming -- this rework is needed for this and also some features that would disable locking on request. So I think we can get this (or something like it) merged very soon

I believe this is now in master from Todd's commits.