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

Consider using double quotes for the values in the metadata.options.environment_variables dict #4836

Closed
bosonie opened this issue Mar 30, 2021 · 5 comments · Fixed by #5349
Closed
Assignees
Milestone

Comments

@bosonie
Copy link

bosonie commented Mar 30, 2021

If one specifies something like:
"environment_variables":{"MY_FILE":"$HOME/path/my_file"},
in the metadata.options, the submission script would include:

# ENVIRONMENT VARIABLES BEGIN ###
export MY_FILE='$HOME/path/my_file'
# ENVIRONMENT VARIABLES  END  ###

This call is ineffective since the $ would be considered a normal character.
The desired behavior would be to return double quotes:

# ENVIRONMENT VARIABLES BEGIN ###
export MY_FILE="$HOME/path/my_file"
# ENVIRONMENT VARIABLES  END  ###

since within double quotes the special character $ is recognized.
For reference, here an extract of the bash manual:

    3.1.2.2 Single Quotes

    Enclosing characters in single quotes (') preserves the literal value of
    each character within the quotes.
    A single quote may not occur between single quotes, even when preceded by
    a backslash.

    3.1.2.3 Double Quotes

    Enclosing characters in double quotes (") preserves the literal value of all characters
    within the quotes, with the exception of $, `, \, and, when history expansion is enabled, !.
    The characters $ and ` retain their special meaning within double quotes (see Shell Expansions).
    The backslash retains its special meaning only when followed by one of the following characters:
    $, `, ", \, or newline. Within double quotes, backslashes that are followed by one of these
    characters are removed. Backslashes preceding characters without a special meaning are left
    unmodified. A double quote may be quoted within double quotes by preceding it with a backslash.
    If enabled, history expansion will be performed unless an ! appearing in double quotes is escaped
    using a backslash. The backslash preceding the ! is not removed.
    The special parameters * and @ have special meaning when in double quotes
    (see Shell Parameter Expansion).

The solution requires some improvements in the method escape_for_bash.

def escape_for_bash(str_to_escape):

@bosonie bosonie added the type/feature request status undecided label Mar 30, 2021
@ramirezfranciscof
Copy link
Member

Hey @bosonie , sorry for the late reply!

I see what you mean; however, I wonder if this might be an intentional decision to avoid (or discourage) environment contamination. For example, I know that we require absolute paths for code setup in order to not rely on the current environment. Thus perhaps the idea is that the environment_variables should be fully defined there, and not depend on external setups.

I'll ask @sphuber and @giovannipizzi for clarification on this.

@sphuber
Copy link
Contributor

sphuber commented Apr 7, 2021

All arguments in all commands in the submission scripts are always single quoted by AiiDA because otherwise they can be misinterpreted. For example, when a path contains spaces, if it is not quoted, it will be interpreted as multiple arguments instead of one. As far as I am aware, this is the main reason for the quoting and not for some security reasons, but this concept has been there from very early versions of AiiDA, so @giovannipizzi should now for sure.

@ramirezfranciscof
Copy link
Member

Ok, so in principle we could add an option to calcjobs to enable the use double commas instead of single commas?

@giovannipizzi
Copy link
Member

Let's say there are two reasons.

  1. one more "ideal": if you inject data from env vars from your machine, you start losing information on how the simulation was actually run (the env-var information is an actual input), so in a sense we lose provenance.
  2. one more practical: one could pass a string as an env var, that even if put in double quotes, is not properly escaped. This can lead to the whole submission script to fail. The idea here was to avoid that passing specific strings could make the whole job fail. [also, while we always try to use bash, in principle this is not always true, the user might be using a different shell with different escaping rules].

In practice, there might be reasons to use env vars from other env vars, anyway. There is at least another issue asking for the same. However, since adding this option might have unexpected results, the question for @bosonie is: is this an issue preventing you to run your simulations properly, or you can work around it?

In any case, having an option that says "set env variables in double quotes rather than single quotes" would be ok with me, as long as it's optional and the default behaviour does not change.

@ltalirz
Copy link
Member

ltalirz commented Apr 21, 2021

I would also vote to support this behavior if it addresses a concrete use case.

unkcpz added a commit to unkcpz/aiida-core that referenced this issue Feb 13, 2022
The escape method will escape the string in the quotes. The defalut
implementation is using single quotes to escape the string. However,
single quotes will not eval the spacial shell parameters such as $ and
@, this makes bash script not versatile to set the parameters in
runtime.

The `use_double_quotes` option is added to escape the string in the
double quotes so the spacial parameters are still valid in the shell
script.

Using double escape option into environment variables of job templete
setting address aiidateam#4836. User can now use double quotes escaping in
calcjob option to escape the environment variables of scheduler.
@sphuber sphuber self-assigned this Feb 13, 2022
@sphuber sphuber added this to the v2.0.0 milestone Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants