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

Fix inconsistent parsing of specs from 'spack env' #7743

Closed

Conversation

alalazo
Copy link
Member

@alalazo alalazo commented Apr 12, 2018

fixes #659

Currently the spack env command fails if more than one token is given for the spec, like:

$ spack env zlib %gcc

This is because the tokens after the first one are interpreted as a command to be executed in the environment.

This PR changes the behavior described above to make it more consistent with that of other commands. Now all of the tokens are interpreted as being part of the spec, and if we want to execute a command in the environment we need to separate it from the spec with '--'.

@alalazo alalazo added bug Something isn't working ready labels Apr 12, 2018
@@ -38,10 +38,16 @@


def setup_parser(subparser):
subparser.epilog = 'To run a command with the emulated environment' \
' insert a \'--\' between the spec and the ' \
Copy link
Member

Choose a reason for hiding this comment

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

Just wrap the string with " instead of ' and you won't need these backslashes.

Copy link
Member Author

@alalazo alalazo Apr 13, 2018

Choose a reason for hiding this comment

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

I guess we have conflicting OCDs. Mixing different styles for strings like:

a = 'a string'
b = "another string"

bothers me a bit (in this case more than escaping 2 characters) 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, change done!

Copy link
Member

Choose a reason for hiding this comment

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

Oh boy, conflicting OCDs is problematic. Sorry for making you suffer! My logic is that Python allows 2 different string delimiters for a reason, and this is the reason! But OCD doesn't respond to logic well, so that might not help you...

fixes spack#659

Currently the `spack env` command fails if more than one token is given
for the spec, like:

spack env zlib %gcc

This is because the tokens after the first one are interpreted as
a command to be executed in the environment.

This PR changes the behavior described above to make it more consistent
with that of other commands. Now all of the tokens are interpreted as
being part of the spec, and if we want to execute a command in the
environment we need to separate it from the spec with '--'.
@alalazo alalazo force-pushed the fixes/spec_parsing_in_env_command branch from fa99981 to 96b8d82 Compare April 13, 2018 07:20
@alalazo
Copy link
Member Author

alalazo commented Apr 13, 2018

@adamjstewart @tgamblin Is the change in behavior fine with you? If so I'll merge the fix.

@tgamblin
Copy link
Member

@alalazo: is this the best way to do this? -- works, but given that spack env only takes one spec, it's actually unambiguous where the spec ends and the command begins. The parser could tell us this -- we could modify parse_one to return the index to start parsing after the first spec encountered.

@alalazo
Copy link
Member Author

alalazo commented Apr 13, 2018

A solution like that probably involves some more care for the error checking part. For instance, adopting the strategy you propose, with this wrong input:

$ spack env zlib szip echo "Hello world!"
==> Error: [Errno 2] No such file or directory

we get an error message that is not very informative. Maintaining the -- we leave the burden of dividing the spec from the command to the user and we can detect without troubles that we have 2 specs. For long specs and long commands the current behavior might even be visually better, as we can spot more easily the -- that divides the two.

OT: where is parse_one again? I can't find it anymore.

@alalazo
Copy link
Member Author

alalazo commented May 21, 2018

ping

@alalazo
Copy link
Member Author

alalazo commented Oct 29, 2019

Any opinion whether this old PR is better to be rebased or closed?

@alalazo alalazo closed this Dec 2, 2019
@alalazo alalazo deleted the fixes/spec_parsing_in_env_command branch December 2, 2019 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent spec parsing in spack env
3 participants