Skip to content
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

Extended set of environment modification commands. #8996

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

skosukhin
Copy link
Member

@skosukhin skosukhin commented Aug 16, 2018

This is yet another attempt (see #6100, #6076, #6652) to extend the set of commands for the environment modifications in compilers.yaml. This time I tried to keep the backward compatibility with the current format of compilers.yaml. Note, that the modifications in schema/compilers.py are currently decorative as the whole file is because it's missing the following lines (see #6652):

         'compilers': {
             'type': 'array',
             'items': {
+                'type': 'object',
+                'additionalProperties': False,
+                'properties': {
                 'compiler': {
                     'type': 'object',
                     'additionalProperties': False,
  • tests
  • documentation

UPD:

  • fixed the yaml schema for compilers.yaml: now it really checks the format

@skosukhin skosukhin force-pushed the compiler_conf_env branch 3 times, most recently from 4d43e65 to fff3f7e Compare August 16, 2018 14:39
@skosukhin
Copy link
Member Author

A failure of the Python 3.7 test suite occurred for unknown reasons but the overall result is positive. Isn't it strange?

@adamjstewart
Copy link
Member

A failure of the Python 3.7 test suite occurred for unknown reasons but the overall result is positive. Isn't it strange?

Python 3.7 tests are allowed to fail in our current build matrix. See https://github.com/spack/spack/blob/develop/.travis.yml#L109.

Copy link
Member

@becker33 becker33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skosukhin overall I like the direction of this. I didn't go far enough with #9005, but since I've been working on this let me know if you want me to help with the changes I've requested.

@@ -698,8 +699,10 @@ without the need to set ``LD_LIBRARY_PATH``.
f77: /opt/gcc/bin/gfortran
fc: /opt/gcc/bin/gfortran
environment:
set:
LD_LIBRARY_PATH : /opt/gcc/lib
- [unset, BAD_VARIABLE]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not continue with the dict representation? It seems easier to represent multiple changes as a dict rather than as a list.

environment:
  set:
    MY_VAR: foo
  unset:
    YOUR_VAR
    LD_PRELOAD
    LIBRARY_PATH
    CPATH
  prepend-path:
    PATH: /opt/gcc/bin
    LD_LIBRARY_PATH: /opt/gcc/lib

Copy link
Member Author

@skosukhin skosukhin Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the order of modifications is unclear if we use a dictionary (even if we use an ordered one, users still have to make sure that it's ordered). What should be the value of MY_VAR if we have the following?

set:
  MY_VAR: /ololo
unset:
  MY_VAR
prepend-path:
  MY_VAR: /alala
append-path:
  MY_VAR: /ilili

Yes, we could just set MY_VAL to the value we need, but why would we need to allow for such confusing constructs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering is good, but I don't like using a list for something that is semantically a map. Since we are essentially associating values with keys, I'd prefer an ordered dict to a list.

[unset, MY_VAR] feels convoluted compared to unset: MY_VAR.

env_cmd = modification[0]
env_cmd_args = modification[1:]
if env_cmd == 'set' and len(env_cmd_args) == 2:
env.set(*env_cmd_args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should also affect the compiler wrapper to ensure nothing in the package prevents them from applying, similar to how the current set semantics work.

See #9005 for how that could work for prepend-path, it shouldn't be hard to adapt that for append-path and unset.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. A package knows better what it needs. @alalazo made a good point regarding this in #5019 (comment). But maybe you could give an example of when this can be helpful? Setting a variable in a wrapper makes a compiler work in an environment different from the environment, in which the configuration takes place, which is really confusing. See another story from @alalazo: #5019 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take @alalazo's point on that. If we're going to go that route, though, we should remove the old variables that aren't needed as well. We can get rid of SPACK_ENV_TO_SET, etc.

@skosukhin skosukhin force-pushed the compiler_conf_env branch 2 times, most recently from e355f12 to ea8dab2 Compare August 31, 2018 17:14
@skosukhin
Copy link
Member Author

Ok, let's keep only the existing format and just extend it. I have also fixed the yaml schema (see the update of the first message). Plus I also removed the SPACK_ENV_TO_SET logic.

@becker33 becker33 merged commit f9617b2 into spack:develop Sep 5, 2018
anderson2981 pushed a commit to anderson2981/spack that referenced this pull request Sep 7, 2018
ptbremer pushed a commit to ptbremer/spack that referenced this pull request Oct 12, 2018
@skosukhin skosukhin deleted the compiler_conf_env branch October 26, 2018 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants