Configuration scopes: fix severe ordering issue when -C and -e are used together#51461
Conversation
Environments are added using ConfigScopePriority.ENVIRONMENT in all cases. Since they are the only kind of scope using that priority, it's safe to activate and deactivate environments. Documentation has been updated to reflect that the CUSTOM scopes have higher priority than ENVIRONMENT. Signed-off-by: Massimiliano Culpo <[email protected]>
alalazo
force-pushed
the
bugfix/big-scope-issue-command-line
branch
from
0e65a80 to
2c61f14
Compare
Priority is not needed, it's always ConfigScopePriority.ENVIRONMENT Signed-off-by: Massimiliano Culpo <[email protected]>
Signed-off-by: Massimiliano Culpo <[email protected]>
Signed-off-by: Massimiliano Culpo <[email protected]>
Signed-off-by: Massimiliano Culpo <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
|
For reference, if we had to keep the positional behavior, the solution would be to organize priorities as: and switch from LOW to HIGH based on where we find |
|
Just quickly tried with my setup and now it looks consistent both the output of |
This comment was marked as outdated.
This comment was marked as outdated.
alalazo
changed the title
-C and -E are used together
alalazo
changed the title
-C and -E are used together-C and -e are used together
This comment was marked as outdated.
This comment was marked as outdated.
|
@tgamblin Is there a reason why (Basically, I'm wondering if in the PR preserving the original intention of #50853 I should also make |
|
After further discussion at the meeting today, I was overruled (which is fine! 😄) . We decided that:
I think the ordering here makes sense even if the CLI order doesn't support ordering across args. And if you want an env to override configuration, you can still supply it with So we can close the other PR and go with this one as-is. Thanks to @alalazo, @becker33, and @scheibelp for their input on this. |
…e used together (spack#51461) Fixes spack#51459. spack#50853 attempted to make configuration (envs and config scopes) specified on the CLI merge in CLI order, but did not quite succeed. Environments, when loaded, would still appear out of order in the scope stack. This fixes the issues and establishes, after much discussion, the ordering we want for configuration scopes specified on the CLI: 1. Ordering of scopes will be enforced *within* the same option on the CLI, e.g. bar is higher precedence in `-C foo -C bar`; 2. Ordering *across* options is counterintuitive, and the `-e` option (or the active environment via `SPACK_ENV`) should always be lower priority than `-C` or `-c` options; and 4. `-C` options have lower priority than `-c`. Environments are now added using `ConfigScopePriority.ENVIRONMENT` in all cases. Since they are the only entity using that scope priority, it's safe to activate and deactivate them. Documentation has been updated to reflect that the CUSTOM scopes have higher priority than ENVIRONMENT. --------- Signed-off-by: Massimiliano Culpo <[email protected]>
|
This PR does not apply cleanly to releases/v1.0 and will be dropped from v1.0.3 |
4 participants
Fixes #51459.
#50853 attempted to make configuration (envs and config scopes) specified on the CLI merge in CLI order, but did not quite succeed. Environments, when loaded, would still appear out of order in the scope stack. This fixes the issues and establishes, after much discussion, the ordering we want for configuration scopes specified on the CLI:
-C foo -C bar;-eoption (or the active environment viaSPACK_ENV) should always be lower priority than-Cor-coptions; and-Coptions have lower priority than-c.Environments are now added using
ConfigScopePriority.ENVIRONMENTin all cases. Since they are the only entity using that scope priority, it's safe to activate and deactivate them.Documentation has been updated to reflect that the CUSTOM scopes have higher priority than ENVIRONMENT.