Improve wheel building and provide ipaserver wheel for local testing #397

tiran · 2017-01-17T11:44:48Z

The PR improve wheel bundle building and allows ipaserver bundles for local testing
with instrumented build of Python. Debug builds and instrumented builds can have a different binary interface (ABI). For example it is useful for dtrace or test installations in a virtual env. ipaplatform and ipaserver will not be uploaded to PyPI, though.

MartinBasti · 2017-01-23T12:09:08Z

ipaserver/plugins/hbactest.py

@@ -210,7 +215,7 @@

 register = Registry()

-def convert_to_ipa_rule(rule):
+def _convert_to_ipa_rule(rule):


This change is not related to pyhbac, should be in separate patch

I made the function a private function because it uses pyhbac internally. Since it's only used internally, I figured out that it makes more sense to make it private than to add a check for pyhbac is not None here, too.

I included a comment in the commit message.

pvoborni · 2017-02-14T16:58:03Z

@tiran I have very vague idea how this is helpful. You have mentioned it during post-devconf "API meeting". But I no longer remember it and description of this PR is very general.

In order to move all the pypi patches forward, we need to document(maybe design) the whole pypi workflow. This is not mentioned in http://www.freeipa.org/page/V4/Build_system_refactoring nor in http://www.freeipa.org/page/V4/Integration_Improvements I.e. how FreeIPA project will work/supply packages to PYPI and what are actually the requirements for these packages. What is expected to work and what not (like everything related to pyhbac).

Right now I have no idea what are the missing blocker parts and what are just nice-to-have things.

Also I don't really like the part that the patches use custom repo of python-nss. But I'm glad that you are working with @jdennis to improve it. @stlaz, with PR #367 what are the remaining usages of python-nss? Could we actually get rid of python-nss completely?

stlaz · 2017-02-15T08:58:13Z

@pvoborni The remaining usages are server/CA certificates verification in certdb.py and apparently some encryption/decryption actions in the Vault plugin. @HonzaCholasta already has patches for the former and getting rid of the latter should not be that hard as well.

tiran · 2017-02-21T09:42:58Z

@pvoborni The main reason for this PR is explained in the initial PR message. I like to run an IPA framework server with specially instrumented Python builds for profiling or for debugging. The special builds are powerful and incredible useful tools to find bugs or hot spots.

Profile and debug builds have a different ABI than standard builds. Therefore I have to compile all C extensions myself to make them compatible with the new ABI. It is much easier than it sounds, because distutils, setuptools and pip just take care of all the complicated bits and pieces. But this works only for native Python packages. SSSD uses its own build system and has no packages on PyPI. It would take too much time and effort to change SSSD now. Commits 1f195bb and c69c30c make pyhbac and other SSSD components optional.

Commit 905118a allows me to build all ipaserver wheel and full ipaclient wheels with install subpackage for local testing. These packages are not meant to be uploaded to PyPI. They are really just for local testing.

Last but not least 5710587 is a workaround for a python-nss packaging issue. @jdennis is aware of the problem and will address it in due time. We can't get rid of python-nss. Dogtag PKI's Python modules depend on python-nss.

tiran · 2017-02-21T09:47:01Z

To clarify and emphasis, this PR has nothing to do with the PyPI packaging effort. Zero. Zip. Nada. Nilch!

The sole intent of this PR is debugging and profiling. It gives us tools to find bugs, to increase performance and to reduce memory usage.

HonzaCholasta · 2017-02-21T12:34:09Z

I can't say I agree with this approach. If this is just for testing, surely you can work around the missing pyhbac in some isolated spot rather than make the import optional all over the place, when it is in fact required. Maybe inject an empty module into sys.modules if it's missing? Or reach out to the SSSD guys and help them add pyhbac to PyPI?

HonzaCholasta · 2017-02-21T12:35:19Z

ipaserver/setup.py

@@ -76,4 +70,12 @@
                'IPASecStore = ipaserver.secrets.store:IPASecStore',
            ],
        },
+        extras_require={
+            # These packages are currently not available on PyPI.
+            "caacl": ["pyhbac"],


The missing pyhbac does not actually affect caacl, but cert - cert-request is the only place where the acl_evaluate function is called.

tiran · 2017-02-22T08:40:18Z

@HonzaCholasta FreeIPA has conditional imports for SSSD modules in several places, e.g. in the trust plugin. 96f614e closes the gap and applies the same technique to the last unconditional import from SSSD.

HonzaCholasta · 2017-02-22T09:16:43Z

The trust plugin and other trust bits are optional. The cert plugin, which depends on pyhbac, is not optional, so you can't apply the same logic to it.

An acceptable compromise would be to skip the cert plugin entirely if pyhbac is not available:

try:
    import pyhbac
except ImportError:
    raise errors.SkipPluginModule(reason=_('pyhbac is not installed'))

tiran · 2017-02-23T08:51:49Z

I didn't know about the SkipPluginModule feature. I agree with you, your solution is more elegant.

tiran · 2017-03-01T08:18:53Z

@jdennis released python-nss 1.0.1. I removed my workaround.

MartinBasti · 2017-03-08T14:05:25Z

needs rebase

HonzaCholasta · 2017-03-09T05:13:24Z

ipaserver/plugins/caacl.py

+try:
+    import pyhbac
+except ImportError:
+    pyhbac = None


SkipPluginModule should be raised here as well. Alternatively, you could move acl_evaluate and this import to cert.py, since that the only place where it's used, and raise SkipPluginModule there.

tiran · 2017-03-14T23:36:43Z

I've moved the code to cert.py and raise SkipPluginModule from there.

tiran · 2017-03-22T10:29:57Z

PR is blocked by #613

MartinBasti · 2017-03-30T15:01:07Z

Please rebase, it is ok to me, I see potential for future server unit testing. I will test when rebased. If somebody is against this please say it now.

HonzaCholasta · 2017-03-31T06:54:07Z

LGTM.

tiran · 2017-03-31T06:57:50Z

Thanks @MartinBasti

I rebased the PR and added a small workaround for dbus-python. The package uses make to compile some of its internal dependencies. It looks like there is a bug in dbus-python's makefile. It sometimes fails to compile with my MAKEFLAGS=-j4 env var. Makefile.am line 253 sets MAKEFLAGS to empty value for pip wheel.

tiran · 2017-03-31T06:59:05Z

Makefile.am

+@MK_ENDIF@
+
+# additional wheels for bundle, e.g. IPA_EXTRA_WHEELS="ipatests[webui] pylint"
+IPA_EXTRA_WHEELS=


I added these two lines to document the intent of IPA_EXTRA_WHEELS.

tiran · 2017-03-31T06:59:23Z

Makefile.am

-	$(PYTHON) -m pip wheel \
+	rm -f $(foreach item,$(IPA_WHEEL_PACKAGES) ipatests,$(WHEELBUNDLEDIR)/$(item)-*.whl)
+	@# dbus-python sometimes fails when MAKEFLAGS is set to -j2 or higher
+	MAKEFLAGS= $(PYTHON) -m pip wheel \


That's the workaround for dbus-python.

MartinBasti · 2017-03-31T10:53:02Z

Build failed:

make wheel_bundle IPA_SERVER_WHEELS=1
...
  checking for DBUS... no
  configure: error: Package requirements (dbus-1 >= 1.6) were not met:
  
  No package 'dbus-1' found
  
  Consider adjusting the PKG_CONFIG_PATH environment variable if you
  installed software in a non-standard prefix.
  
  Alternatively, you may set the environment variables DBUS_CFLAGS
  and DBUS_LIBS to avoid the need to call pkg-config.
  See the pkg-config man page for more details.
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
    File "/tmp/pip-build-l97uxR/dbus-python/setup.py", line 106, in <module>
      'build_ext': BuildExt,
    File "/usr/lib64/python2.7/distutils/core.py", line 151, in setup
      dist.run_commands()
    File "/usr/lib64/python2.7/distutils/dist.py", line 953, in run_commands
      self.run_command(cmd)
    File "/usr/lib64/python2.7/distutils/dist.py", line 972, in run_command
      cmd_obj.run()
    File "/usr/lib/python2.7/site-packages/wheel/bdist_wheel.py", line 199, in run
      self.run_command('build')
    File "/usr/lib64/python2.7/distutils/cmd.py", line 326, in run_command
      self.distribution.run_command(command)
    File "/usr/lib64/python2.7/distutils/dist.py", line 972, in run_command
      cmd_obj.run()
    File "/tmp/pip-build-l97uxR/dbus-python/setup.py", line 62, in run
      cwd=builddir)
    File "/usr/lib64/python2.7/subprocess.py", line 186, in check_call
      raise CalledProcessError(retcode, cmd)
  subprocess.CalledProcessError: Command '['/tmp/pip-build-l97uxR/dbus-python/configure', '--disable-maintainer-mode', 'PYTHON=/usr/bin/python', '--prefix=/tmp/pip-build-l97uxR/dbus-python/build/temp.linux-x86_64-2.7/prefix']' returned non-zero exit status 1
  
  ----------------------------------------
  Failed building wheel for dbus-python
  Running setup.py clean for dbus-python
  Running setup.py bdist_wheel for MarkupSafe ... done
  Stored in directory: /tmp/freeipa/dist/bundle
  Running setup.py bdist_wheel for pycparser ... done
  Stored in directory: /tmp/freeipa/dist/bundle
  Running setup.py bdist_wheel for configparser ... done
  Stored in directory: /tmp/freeipa/dist/bundle
Successfully built cryptography python-yubico pyusb python-nss pyldap netifaces gssapi MarkupSafe pycparser configparser
Failed to build dbus-python
ERROR: Failed to build one or more wheels
Makefile:1222: recipe for target 'wheel_bundle' failed

tiran · 2017-03-31T11:34:57Z

You need dbus-devel package.

I opened https://pagure.io/freeipa/issue/6842 to track lack of documentation.

MartinBasti · 2017-03-31T11:47:29Z

So put it into specfile to with_wheels section

MartinBasti · 2017-03-31T11:47:59Z

And document in BUILD.txt how to build wheels

tiran · 2017-03-31T14:29:16Z

@MartinBasti dbus-devel is in the with_wheels section. Documentation is part of https://pagure.io/freeipa/issue/6842 .

MartinBasti · 2017-04-03T07:35:32Z

@tiran sorry, but then something doesn't work as expected

$ dnf builddep -b -D "with_wheels 1" --spec freeipa.spec.in
<everything already installed>

$ make wheel_bundle IPA_SERVER_WHEELS=1
...
  Failed building wheel for dbus-python

$ git grep -in  dbus-devel freeipa.spec.in 
freeipa.spec.in:149:BuildRequires:  dbus-devel

$ rpm -q dbus-devel
package dbus-devel is not installed

Probably because %global with_wheels 0 is defined in the spec file, so it always overrides my CLI settings

MartinBasti · 2017-04-03T08:28:40Z

Missing dependency dbus-glib-devel otherwise it works

tiran · 2017-04-03T09:23:47Z

Sigh, I got it.

MartinBasti · 2017-04-03T09:49:59Z

Can you rebase please? I see Ben's commits in this PR

The pyhbac module is part of SSSD. It's not available as stand-alone PyPI package. It would take a lot of effort to package it because the code is deeply tight into SSSD. Let's follow the example of other SSSD Python packages and make the import of pyhbac conditionally. It's only necessary for caacl and hbactest plugins. I renamed convert_to_ipa_rule() to _convert_to_ipa_rule() because it does not check for presence of pyhbac package itself. The check is performed earlier in execute(). The prefix indicates that it is an internal function and developers have to think twice before using it in another place. This makes it much easier to install ipaserver with instrumented build of Python with a different ABI or in isolated virtual envs to profile and debug the server. Signed-off-by: Christian Heimes <cheimes@redhat.com>

ipaserver did not have extra_requires to state additional dependencies. Signed-off-by: Christian Heimes <cheimes@redhat.com>

To create a wheel bundle with ipaserver and its dependencies: make wheel_bundle IPA_SERVER_WHEELS=1 To include additional dependencies: make wheel_bundle IPA_EXTRA_WHEELS=ipatests[webui] Signed-off-by: Christian Heimes <cheimes@redhat.com>

Signed-off-by: Christian Heimes <cheimes@redhat.com>

MartinBasti · 2017-04-03T11:09:08Z

master:

3064b89 Conditionally import pyhbac
7c9df35 Add extra_requires for additional dependencies
ae1c208 Add an option to build ipaserver wheels
40a6067 Don't hard-code with_wheels

MartinBasti self-assigned this Jan 20, 2017

MartinBasti suggested changes Jan 23, 2017

View reviewed changes

tiran force-pushed the ipaserver_bundle branch from d17ff7e to 5710587 Compare January 23, 2017 13:34

HonzaCholasta reviewed Feb 21, 2017

View reviewed changes

tiran force-pushed the ipaserver_bundle branch from 5710587 to c505a31 Compare February 21, 2017 16:13

tiran force-pushed the ipaserver_bundle branch 2 times, most recently from e51fe8b to ab6d1f1 Compare March 1, 2017 08:18

HonzaCholasta reviewed Mar 9, 2017

View reviewed changes

tiran force-pushed the ipaserver_bundle branch from ab6d1f1 to cc78282 Compare March 14, 2017 23:35

tiran force-pushed the ipaserver_bundle branch from cc78282 to 5ef5839 Compare March 16, 2017 06:45

MartinBasti approved these changes Mar 30, 2017

View reviewed changes

tiran force-pushed the ipaserver_bundle branch from 5ef5839 to 1348a31 Compare March 31, 2017 06:50

tiran commented Mar 31, 2017

View reviewed changes

tiran force-pushed the ipaserver_bundle branch from 1348a31 to 5f48cb1 Compare March 31, 2017 13:28

tiran force-pushed the ipaserver_bundle branch 2 times, most recently from 8764e8d to 60f2d3c Compare April 3, 2017 08:04

tiran force-pushed the ipaserver_bundle branch from 60f2d3c to 31d4010 Compare April 3, 2017 08:41

tiran added 4 commits April 3, 2017 12:14

Add extra_requires for additional dependencies

c47c42e

ipaserver did not have extra_requires to state additional dependencies. Signed-off-by: Christian Heimes <cheimes@redhat.com>

Add an option to build ipaserver wheels

2f74325

To create a wheel bundle with ipaserver and its dependencies: make wheel_bundle IPA_SERVER_WHEELS=1 To include additional dependencies: make wheel_bundle IPA_EXTRA_WHEELS=ipatests[webui] Signed-off-by: Christian Heimes <cheimes@redhat.com>

Don't hard-code with_wheels

c8c9450

Signed-off-by: Christian Heimes <cheimes@redhat.com>

tiran force-pushed the ipaserver_bundle branch 2 times, most recently from 9b753e5 to c8c9450 Compare April 3, 2017 10:37

MartinBasti added the ack Pull Request approved, can be merged label Apr 3, 2017

MartinBasti added the pushed Pull Request has already been pushed label Apr 3, 2017

MartinBasti closed this Apr 3, 2017

tiran deleted the ipaserver_bundle branch April 4, 2017 07:33

Improve wheel building and provide ipaserver wheel for local testing #397

Improve wheel building and provide ipaserver wheel for local testing #397

tiran commented Jan 17, 2017 •

edited by MartinBasti

MartinBasti Jan 23, 2017

tiran Jan 23, 2017

tiran Jan 23, 2017

pvoborni commented Feb 14, 2017

stlaz commented Feb 15, 2017 •

edited

tiran commented Feb 21, 2017

tiran commented Feb 21, 2017

HonzaCholasta commented Feb 21, 2017

HonzaCholasta Feb 21, 2017

tiran Feb 21, 2017

tiran commented Feb 22, 2017

HonzaCholasta commented Feb 22, 2017

tiran commented Feb 23, 2017

tiran commented Mar 1, 2017

MartinBasti commented Mar 8, 2017

HonzaCholasta Mar 9, 2017

tiran commented Mar 14, 2017

tiran commented Mar 22, 2017

MartinBasti commented Mar 30, 2017

HonzaCholasta commented Mar 31, 2017

tiran commented Mar 31, 2017

tiran Mar 31, 2017

tiran Mar 31, 2017

MartinBasti commented Mar 31, 2017

tiran commented Mar 31, 2017

MartinBasti commented Mar 31, 2017

MartinBasti commented Mar 31, 2017

tiran commented Mar 31, 2017

MartinBasti commented Apr 3, 2017

MartinBasti commented Apr 3, 2017

tiran commented Apr 3, 2017

MartinBasti commented Apr 3, 2017

MartinBasti commented Apr 3, 2017

Improve wheel building and provide ipaserver wheel for local testing #397

Improve wheel building and provide ipaserver wheel for local testing #397

Conversation

tiran commented Jan 17, 2017 • edited by MartinBasti

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvoborni commented Feb 14, 2017

stlaz commented Feb 15, 2017 • edited

tiran commented Feb 21, 2017

tiran commented Feb 21, 2017

HonzaCholasta commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Feb 22, 2017

HonzaCholasta commented Feb 22, 2017

tiran commented Feb 23, 2017

tiran commented Mar 1, 2017

MartinBasti commented Mar 8, 2017

Choose a reason for hiding this comment

tiran commented Mar 14, 2017

tiran commented Mar 22, 2017

MartinBasti commented Mar 30, 2017

HonzaCholasta commented Mar 31, 2017

tiran commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti commented Mar 31, 2017

tiran commented Mar 31, 2017

MartinBasti commented Mar 31, 2017

MartinBasti commented Mar 31, 2017

tiran commented Mar 31, 2017

MartinBasti commented Apr 3, 2017

MartinBasti commented Apr 3, 2017

tiran commented Apr 3, 2017

MartinBasti commented Apr 3, 2017

MartinBasti commented Apr 3, 2017

tiran commented Jan 17, 2017 •

edited by MartinBasti

stlaz commented Feb 15, 2017 •

edited