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

RFR: Add --always-copy option to create_virtualenv method for pack virtual… #2372

Merged
merged 6 commits into from Jan 13, 2016

Conversation

lakshmi-kannan
Copy link
Contributor

…envs

See StackStorm/st2-packages#53 for details.

I anyways added config options for virtualenv_binary and virtualenv_opts in case we want to play around with these in the future so it's easier. The only legit change that fixed anything was --always-copy.

This works on my dev env as well. Meanwhile, @Kami, @DoriftoShoes, @dennybaa and @armab please take a look at my comment on StackStorm/st2-packages#53 and this change.

TODO

  • Docs changes --> Not sure where to document this!
  • Changelog

@estee-tew
Copy link

@estee-tew
Copy link

try:
exit_code, _, stderr = run_command(cmd=cmd)
except OSError:
raise Exception('Virtualenv binary "%s" doesn\'t exist.' % virtualenv_binary)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would also include original error message at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original error message was "No such file or directory" without naming the file. This was confusing. Thus this new message.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

But OSError doesn't necessary only mean that the file doesn't exist, it could also represent a permissions error, etc. That's why I think it's also good to include the original message somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let me log the original exception and raise this new one. Does that sound good to you?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to include it in the exception message, but whatever makes you happy :P

@lakshmi-kannan
Copy link
Contributor Author

Validated with this that sensors and actions are able to run fine with the new packages. Also, this opened up another issue https://stackstorm.atlassian.net/browse/STORM-1949 . cc: @dennybaa @Kami

@manasdk
Copy link
Contributor

manasdk commented Jan 13, 2016

Yup --always-copy worked for me as well.

@lakshmi-kannan lakshmi-kannan changed the title WIP: Add --always-copy option to create_virtualenv method for pack virtual… RFR: Add --always-copy option to create_virtualenv method for pack virtual… Jan 13, 2016
@dennybaa
Copy link
Contributor

+1 to merge. One thing --system-site-packages should be removed, since it's not required anymore due to injection to PYTHONPATH.

@lakshmi-kannan
Copy link
Contributor Author

@dennybaa Unfortunately, we cannot remove --system-site-packages from the code because old packages still need them. See https://stackstorm.slack.com/archives/stackstorm/p1452708198024063

So in st2-packages, can you set the virtualenv_opts = ['--always-copy'] in /etc/st2/st2.conf. This way new packages won't use --system-site-packages.

Lakshmi Kannan added 3 commits January 13, 2016 10:09
* master:
  Add missing fixture file.
  Fix lint.
  Update changelog.
  Add a test case for it.
  Throw a more friendly exception if casting a particular parameter value fails.
  Change shebang in new st2ctl
  Include st2ctl as part of st2common

Conflicts:
	CHANGELOG.rst
@lakshmi-kannan
Copy link
Contributor Author

Seems like travis is stuck. I'll merge and deal with build failures if any.

lakshmi-kannan added a commit that referenced this pull request Jan 13, 2016
RFR: Add --always-copy option to create_virtualenv method for pack virtual…
@lakshmi-kannan lakshmi-kannan merged commit 48ae45e into StackStorm:master Jan 13, 2016
@lakshmi-kannan lakshmi-kannan deleted the virtualenv_fix branch January 13, 2016 19:28
@lakshmi-kannan
Copy link
Contributor Author

See StackStorm/puppet-st2#148. Basically, --always-copy fails with RHEL boxes (pypa/virtualenv#565 (comment)). So I am setting a config option for existing installation to just use --system-site-packages. RHEL st2bundle won't work with --always-copy but @DoriftoShoes and I decided to deal with that later.

@lakshmi-kannan
Copy link
Contributor Author

If I don't get workroom to pass with StackStorm/puppet-st2#148, I'll revert this change from master for 1.3 release. cc: @Kami, @DoriftoShoes, @emedvedev, @dennybaa.

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.

None yet

5 participants