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

Increased M2Crypto to 0.30.1; Used M2Crypto on Windows #1248

Merged
merged 1 commit into from Jun 7, 2018

Conversation

andy-maier
Copy link
Contributor

@andy-maier andy-maier commented May 11, 2018

For details, see the commit message.
Ready for review and merge.

For the review:

  • Try it out on Windows
  • Review the whole install process we now have on Windows (see install section)

@coveralls
Copy link

coveralls commented May 11, 2018

Coverage Status

Coverage remained the same at 84.724% when pulling 3576940 on andy/new-m2crypto into bb3659f on master.

@andy-maier andy-maier force-pushed the andy/new-m2crypto branch 4 times, most recently from a13ef0a to eed9da3 Compare May 11, 2018 16:26
@andy-maier andy-maier changed the title Increased M2Crypto minimum version to 0.30.0 [WIP] Increased M2Crypto minimum version to 0.30.0 May 11, 2018
@andy-maier andy-maier force-pushed the andy/new-m2crypto branch 16 times, most recently from 3c08754 to 4a823fb Compare May 13, 2018 10:11
@andy-maier
Copy link
Contributor Author

Rebased.

@KSchopmeyer
Copy link
Collaborator

Note: Since in the Readme we state that it is supported on python 2 and 3 we really need to clarify the readme.

appveyor.yml Outdated
# The installation in pywbem_os_setup.bat installs swig but without generating
# the swig.exe shim file (using GenShim).
# See GenShim issue https://github.com/chocolatey/shimgen/issues/43
# Therefore, we install swig ihere in order to create the swig.exe shim file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is word ihere??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"here" with an "i" for vi insert mode ;-)
DONE.

Copy link
Collaborator

@KSchopmeyer KSchopmeyer left a comment

Choose a reason for hiding this comment

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

Also, right not the readme states:

Pywbem is a WBEM client and WBEM indication listener, written in pure Python. It runs on Python 2 and Python 3.

We need to clalrify that. Also we never stated which OS's it supports in the readme.

Finally, the readme shows the linux install and does not reference windows at all or indicate that there are any differences. Maybe time to point them to the detailed install for windows since that is what we really maintain.

@@ -108,7 +108,7 @@ Pywbem is supported in these environments:
Limitations:

* On Windows (native), pywbem is not supported on Python 2.6, because the
M2CryptoWin32/64 packages do not support Python 2.6.
M2Crypto package does not support Python 2.6.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean the M2Crypto windows package???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR changes from using the outdated M2CryptoWin32/64 packages to using the M2Crypto package, on Windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So should say that M2Crypto package on Windows does not support ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what it says, if you read the whole sentence.

@@ -268,6 +263,8 @@ environment (i.e. without using a UNIX-like environment; for that, see

> pip install pywbem

.. _`Chocolatey package manager`: https://chocolatey.org/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we say more than just Chocolatey package manager and a url???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just the definition of a link target; it does not generate anything in the text. It is used a few lines up.

@KSchopmeyer
Copy link
Collaborator

I reviewed the files changes but want to go back and completely read the doc. Also I am fighting chocolatey right now on my windows install and will update you when I get through that.

@KSchopmeyer
Copy link
Collaborator

Which will be slow since windows just decided to do a major update.

Copy link
Collaborator

@KSchopmeyer KSchopmeyer left a comment

Choose a reason for hiding this comment

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

The readme just states python 2 and 3 but nothing about OS, etc.

Also in the readme we show a simple pip install on linux but just refer to the install instructions for anything else. I really don't think we want to duplicate the doc install but should probably clarify a bit more including the issue that python 2.6 windows is a NoNo

@andy-maier
Copy link
Contributor Author

andy-maier commented Jun 4, 2018

On updating the README: I don't think we should update the README to duplicate the complete information from the Installation section. I have updated the README to state that the installation method shown is for Linux and that other operating systems are described in the installation section.

I agree. What I was thinking was that we should state that we support on Linux, Windows, MAC Os and not that Windows python 2.6 is not supported because of M2Crypto since those are major characteristics and limits. Leave the example as it and refer to doc for everything else.

@andy-maier
Copy link
Contributor Author

Open at this point for Karl:

  • Review the complete installation section of the documentation
  • Verify the documented installation approach on Windows

@KSchopmeyer
Copy link
Collaborator

KSchopmeyer commented Jun 5, 2018

Testing with windows. This is fun.

  1. Found that the terminal has to be in admin mode since chocolatey accesses data in the C:/program files ... directory and that fails without admin mode. That, of course means a difference setup since this is not logical like sudo.
    AM> DONE, added to use admin command prompt.

  2. pywbem_os-setup.bat requires a parameter "develop" or "install" and that is not documented.
    AM> DONE, added default of install when nothing is specified.

  3. Got through that and got it to build m2crypto and hit the following issue:

    Creating library build\temp.win-amd64-2.7\Release\SWIG_m2crypto.lib and object build\temp.win- amd64-2.7\Release\SWIG_m2crypto.exp
    python setup.py bdist_wheel
    usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
    or: setup.py --help [cmd1 cmd2 ...]
    or: setup.py --help-commands
    or: setup.py cmd --help

error: invalid command 'bdist_wheel'
Error: Command returned rc=1
AM> DONE, added an according trouble shooting entry.

Quit for night at that point

That was easy to fix. wheel not installed apparently. did pip install wheel and it worked.

@KSchopmeyer
Copy link
Collaborator

KSchopmeyer commented Jun 5, 2018

New comments:

Made it all the way through a pip install and clone download etc. on windows and tested by running wbemcli.

  1. I propose that we change "and it is assumed that Chocolatey is installed" in the windows install section to "Chocolatey must be installed before running the above script." Just want to make that stand out since it is a PIA to do.
    AM> DONE - Moved that into a new "prerequisites" item. I inserted such an item for all target platforms.

  2. The script asks that either "develop" or "install" keywords be on the cmd line. a)That probably deserves more explanation and b) that should be in the doc.
    AM> DONE - Changed .bat script to default to install, consistent with the .sh version.

  3. I could not checkout all the links in the doc (ex. to pywbem_os_bat) because in the local builddoc they are broken.
    AM> Only the download links cannot be checked out locally, but I checked them for the .sh version, and used the same approach for the .bat version, so I'm confident they will work.

I cannot explain some of the fits and starts in testing but it did work out when I cleaned everything out and started over.

I think with these comments it is ready to commit. Then, however, we should go through the testing again since this is so many lose parts all sort of flying in formation.

Congratulations on getting this one sorted out

@andy-maier
Copy link
Contributor Author

This last commit is supposed to address all comments that were made.
If anything is not addressed, please make new comments.

@andy-maier
Copy link
Contributor Author

I had to do a small fix with directory switching in pywbem_os_setup.bat

M2Crypto 0.29.0 introduced support for Windows again, so it is no longer
necessary to use the very outdated M2CryptoWin32/64 packages.

This change uses the M2Crypto package now also on Windows, and eliminates
using M2CryptoWin32/64.

To enable that, it increases the minimum version of M2Crypto to 0.30.1
(which has a fix for the Windows support introduced with 0.29.0).

It also adds a pywbem_os_setup.bat script which installs OS-level dependencie
for installing M2Crypto (e.g. its build requires swig, Win32/64OpenSSL).

Addressed review comments

* Updated README.rst file to scope shown installation approach to Linux.
* Typo in appveyor.yml file.

Addressed review comments (2)

- Added prerequisites such as initial python packages, etc.
  to the installation sections of each platform.
- The pywbem_os_setup.bat file got extended to perform install
  as a default when no argument is specified, consistent with
  the .-sh version.

Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
@andy-maier andy-maier merged commit fb19a89 into master Jun 7, 2018
@andy-maier andy-maier deleted the andy/new-m2crypto branch June 7, 2018 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants