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 extra spaces and colons in flags and environment variables #8496

Merged
merged 6 commits into from
Feb 23, 2021

Conversation

klimkin
Copy link
Contributor

@klimkin klimkin commented Feb 12, 2021

According to Bash expansion:

${parameter:+word}

Correct form for PATH:

${PATH:+:$PATH}

Correct form for CFLAGS:

${CFLAGS:+ $CFLAGS}

Fixes #8495

Changelog: Fix: Remove extra spaces in flags and colons in path variables.
Docs: omit

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

According to
[Bash expansion](https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html):

    ${parameter:+word}

Correct form for PATH:

    ${PATH:+:$PATH}

Correct form for CFLAGS:

    ${CFLAGS:+ $CFLAGS}

Fixes conan-io#8495
@CLAassistant
Copy link

CLAassistant commented Feb 12, 2021

CLA assistant check
All committers have signed the CLA.

@klimkin
Copy link
Contributor Author

klimkin commented Feb 13, 2021

@jgsogo, @memsharded FYI

@memsharded memsharded added this to the 1.34 milestone Feb 13, 2021
@memsharded
Copy link
Member

Thanks for contributing this @klimkin

I am a bit surprised that our tests didn't catch such bug. I am experimenting with a new way to handle the environment in #8426, and I haven't been able to reproduce, it seems that modern bash can process the expansion correctly without the :.

I think this is most likely good to be merged, if it is the "canonical" way, but it would be great to have also first a failing test that proves that the current expansion without ":" doesn't work as intended. Could you please try to contribute such a test, or give some hints about the failures that could happen if the ":" is missing?

@klimkin
Copy link
Contributor Author

klimkin commented Feb 13, 2021

Added unit-test: e64fee7

@memsharded Here is a nice write-up: https://tldp.org/LDP/abs/html/parameter-substitution.html

{parameter+alt_value}, ${parameter:+alt_value}
If parameter set, use alt_value, else use null string.

Both forms nearly equivalent. The : makes a difference only when parameter has been declared and is null, see below.

param=

a=${param+xyz}
echo "a = $a"      # a = xyz

a=${param:+xyz}
echo "a = $a"      # a =
# Different result from   a=${param5+xyz}

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Love to see all those FIXME being actually fixed! 💯

conans/test/functional/generators/virtualenv_test.py Outdated Show resolved Hide resolved
conans/test/functional/generators/virtualenv_test.py Outdated Show resolved Hide resolved
@memsharded memsharded merged commit bf4fe85 into conan-io:develop Feb 23, 2021
@klimkin klimkin deleted the hotfix/extra-space-and-colon branch March 27, 2021 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Path expansion in virtualrunenv generator adds extra colon for empty path
4 participants