Merge lp:~vila/bzr/385453-make-pyrex into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

So this implements a simple-minded fix for the bug.

The idea is that setup.py is used, so far, to either build extensions or build the distribution which should always include the extensions anyway.

In short, setup is useless if you don't have pyrex or if you don't have the C files.

So the implemented fix make setup abort if pyrex is not available *and* the C files are not available.

It also starts implementing tests for that.

As discussed with Martin and John, splitting the build step into generation and compilation and
archiving the post-processed generated files can come later.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/385453-make-pyrex into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> So this implements a simple-minded fix for the bug.
>
> The idea is that setup.py is used, so far, to either build extensions or build the distribution which should always include the extensions anyway.
>
> In short, setup is useless if you don't have pyrex or if you don't have the C files.
>
> So the implemented fix make setup abort if pyrex is not available *and* the C files are not available.
>
> It also starts implementing tests for that.
>
> As discussed with Martin and John, splitting the build step into generation and compilation and
> archiving the post-processed generated files can come later.
>

This seems to completely disable --allow-python-fallbacks which is how
we handle it in the past.

Am I missing something?

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpBCjYACgkQJdeBCYSNAAN7uQCcD1ZKQpqr7R4jOIKwyCU+EEe6
ucwAoMK3Eskll0CWf3J+4MzdOYjxSMF7
=1bEf
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Vincent Ladeuil wrote:
    >> Vincent Ladeuil has proposed merging lp:~vila/bzr/385453-make-pyrex into lp:bzr.
    >>
    >> Requested reviews:
    >> bzr-core (bzr-core)
    >>
    >> So this implements a simple-minded fix for the bug.
    >>
    >> The idea is that setup.py is used, so far, to either build extensions or build the distribution which should always include the extensions anyway.
    >>
    >> In short, setup is useless if you don't have pyrex or if you don't have the C files.
    >>
    >> So the implemented fix make setup abort if pyrex is not available *and* the C files are not available.
    >>
    >> It also starts implementing tests for that.
    >>
    >> As discussed with Martin and John, splitting the build step into generation and compilation and
    >> archiving the post-processed generated files can come later.
    >>

    jam> This seems to completely disable --allow-python-fallbacks which is how
    jam> we handle it in the past.

    jam> Am I missing something?

Grr, no, I missed to ask the question that prompted me to submit:

What is the intended use of --allow-python-fallbacks ?

As far as I can tell, it is to allow compile errors to be
ignored, so failing to build the extensions is not seen as an
error to build a distribution. I think we want to get away from
such tolerance, the bug title is:
"make dist should fail if C files don't exist or can't be built"

That's why I summarize above: "the distribution should always
include the extensions".

I confess that the setup.py is a bit hard to analyze without
being able to test it :-/

I'd say that at least 2/3 of it is windows specific... It would
be nice to define more specific setup commands for each sub part
(even if not really needed from a pure setup point of view, if
only to be able to test them and use them for make purposes), the
current relationship between setup and Makefile are a bit hard to
follow. Anyway, that's outside the scope of this bug.

So this submission is more RFC than MERGE.

[Martin Pool (mbp)]

2009/6/24 Vincent Ladeuil <email address hidden>:
>>>>>> "jam" == John A Meinel <email address hidden> writes:
>
> Grr, no, I missed to ask the question that prompted me to submit:
>
> What is the intended use of --allow-python-fallbacks ?
>
> As far as I can tell, it is to allow compile errors to be
> ignored, so failing to build the extensions is not seen as an
> error to build a distribution. I think we want to get away from
> such tolerance, the bug title is:
> "make dist should fail if C files don't exist or can't be built"
>
> That's why I summarize above: "the distribution should always
> include the extensions".

As I understand it the goal is that you can run bzr from source even
without a compiler. I think this is a bit useful for people
installing on a minority or difficult operating system.

I don't think it needs to be a supported or even desirable case to
allow you to make a tarball with no .c files, or even to make a binary
package without the built libraries.

I could even see a case for saying in that extreme case you need to
just run from the source directory and not run setup.py at all. Maybe
it's not important at all; if you're running python at all obviously
someone somewhere has a compiler that can build Python.

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

On Thu, 2009-06-25 at 08:57 +0000, Martin Pool wrote:
>
> I could even see a case for saying in that extreme case you need to
> just run from the source directory and not run setup.py at all. Maybe
> it's not important at all; if you're running python at all obviously
> someone somewhere has a compiler that can build Python.

The compiler may be a cross-compiler; theres no strong case for having a
native compiler. Perhaps this is overly pessimistic.

-Rob

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

> As I understand it the goal is that you can run bzr from source even
> without a compiler. I think this is a bit useful for people
> installing on a minority or difficult operating system.

The idea was that you could run/install bzr even without having a
compiler. This is somewhat useful for people who are deploying bzr
manually on machines that they don't want to install the compiler tool
chain.

>
> I don't think it needs to be a supported or even desirable case to
> allow you to make a tarball with no .c files, or even to make a binary
> package without the built libraries.
>
> I could even see a case for saying in that extreme case you need to
> just run from the source directory and not run setup.py at all. Maybe
> it's not important at all; if you're running python at all obviously
> someone somewhere has a compiler that can build Python.
>

If we want to change the story to "you can run from source without
compiling extensions, but we won't let you install" I can be okay with
that, but we should be clear that *that* is what we are now saying.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpDioEACgkQJdeBCYSNAAPkdgCfSgMVA5UNdWj5Yr2iIrIdme9N
Pr4AmQHfF5nvvRV9HRaAVB5Na3NnblPZ
=jTob
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

> So this implements a simple-minded fix for the bug.
>
> The idea is that setup.py is used, so far, to either build extensions or build
> the distribution which should always include the extensions anyway.
>
> In short, setup is useless if you don't have pyrex or if you don't have the C
> files.
>
> So the implemented fix make setup abort if pyrex is not available *and* the C
> files are not available.

So, to try to finish this review off:

I think we want this behaviour:

if allow_python_fallbacks:
  pass
else:
  if not have_current_c_files:
    must_have_pyrex
  must_have_cc

This should apply to installation, and to making source and binary distributions.

Then 'make dist' should not pass --allow-python-fallbacks, so someone trying to prepare an official release should always include them.

I think this is consistent with what John and Robert are saying.

I don't think this is what this patch does, so for the sake of keeping the queue clean I'm going to say resubmit. But if you think we should have different behaviour we can certainly talk about it.

70 # TODO: Run bzr from the installed copy to see if it works. Really we need to
71 # run something that exercises every module, just starting it may not detect
72 # some missing modules.
73 #
74 +# NOTE: Doing the above for every commit seems overkill. I'm not against
75 +# defining a separate test uite for precisely that purpose to be run at
76 +# *release* time, but then, our actual test suite sounds adequate
77 +# -- vila 20090623
78 +#
79 # TODO: Check that the version numbers are in sync. (Or avoid this...)

I think the previous comment is now mostly obsolete, as we make 'make dist-check-install', and running the test suite is a recommended part of the release process. I suggest you just delete the comment; if there are failures where things are not being properly installed we can look for cheaper ways to do it.

review: Needs Fixing

Unmerged revisions

4465. By Vincent Ladeuil on 2009-06-23

Rough fix for bug#385453.

* setup.py:
Some cleanup and exit early if pyrex generated C files are not
available.

* bzrlib/tests/test_setup.py:
(TestSetup.setUp): CleanupBetter protect the build dir.
(TestSetupBuild): Start adding tests for the build step.
(TestSetupInstall): Separate install step tests.

* bzrlib/tests/__init__.py:
(PyrexFeature): Most extensions use pyrex.

4464. By Vincent Ladeuil on 2009-06-23

Cleanup setup tests.

* bzrlib/tests/test_setup.py:
(TestSetup): Protect the existing build dir.
(TestSetup.test_build_and_install.rmtree): Use cleanup hooks and
don't clobber the existing build dir.

* .bzrignore:
All pyrex generated files follow the same pattern.

4463. By Vincent Ladeuil on 2009-06-23

Fixed as per John and Martin reviews.

* bzrlib/_dirstate_helpers_pyx.pyx:
(_cmp_path_by_dirblock_intern): Renamed as requested.

4462. By Vincent Ladeuil on 2009-06-22

Fix failing benchmarks for dirstate helpers.

4461. By Vincent Ladeuil on 2009-06-22

Use the same method or function names for _dirstate_helpers in pyrex and
python modules.

4460. By Vincent Ladeuil on 2009-06-22

Use a consistent scheme for naming pyrex source files.

4459. By Canonical.com Patch Queue Manager <email address hidden> on 2009-06-18

(jam) Remove a spurious call to _get_raw_record,
 speeds up get_record_stream(..., True)

4458. By Canonical.com Patch Queue Manager <email address hidden> on 2009-06-18

(andrew) Fix branch format upgrades triggered by default stacking
 policy on a smart server.

4457. By Canonical.com Patch Queue Manager <email address hidden> on 2009-06-18

(igc) fix ls DIR --from-root and improve ls performance

4456. By Canonical.com Patch Queue Manager <email address hidden> on 2009-06-17

(jam) Fix some failing tests on win32

=== modified file '.bzrignore'
--- .bzrignore	2009-06-22 12:52:39 +0000
+++ .bzrignore	2009-07-01 22:35:49 +0000
@@ -38,16 +38,8 @@
 ./api
 doc/**/*.html
 doc/developers/performance.png
-bzrlib/_bencode_pyx.c
-bzrlib/_btree_serializer_pyx.c
-bzrlib/_chk_map_pyx.c
-bzrlib/_chunks_to_lines_pyx.c
-bzrlib/_dirstate_helpers_pyx.c
-bzrlib/_groupcompress_pyx.c
-bzrlib/_knit_load_data_pyx.c
-bzrlib/_known_graph_pyx.c
-bzrlib/_readdir_pyx.c
-bzrlib/_rio_pyx.c
+# Pyrex generated C files
+bzrlib/_*_pyx.c
 bzrlib/_walkdirs_win32.c
 doc/en/release-notes/NEWS.txt
 doc/en/developer-guide/HACKING.txt

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-06-29 15:25:08 +0000
+++ bzrlib/tests/__init__.py	2009-07-01 22:35:49 +0000
@@ -3986,3 +3986,20 @@
             return result
 except ImportError:
     pass
+
+
+class _PyrexFeature(Feature):
+
+    def _probe(self):
+        try:
+            import Pyrex
+            return True
+        except ImportError:
+            return False
+
+    def feature_name(self):
+        return 'pyrex'
+
+PyrexFeature = _PyrexFeature()
+
+

=== modified file 'bzrlib/tests/test_setup.py'
--- bzrlib/tests/test_setup.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_setup.py	2009-07-01 22:35:49 +0000
@@ -20,55 +20,47 @@
 import sys
 import subprocess
 import shutil
-from tempfile import TemporaryFile
+import tempfile
 
 import bzrlib
-from bzrlib.tests import TestCase, TestSkipped
-import bzrlib.osutils as osutils
+from bzrlib import (
+    osutils,
+    tests,
+    )
 
-# XXX: This clobbers the build directory in the real source tree; it'd be nice
-# to avoid that.
-#
 # TODO: Run bzr from the installed copy to see if it works.  Really we need to
 # run something that exercises every module, just starting it may not detect
 # some missing modules.
 #
+# NOTE: Doing the above for every commit seems overkill. I'm not against
+# defining a separate test uite for precisely that purpose to be run at
+# *release* time, but then, our actual test suite sounds adequate
+# -- vila 20090623
+#
 # TODO: Check that the version numbers are in sync.  (Or avoid this...)
 
-class TestSetup(TestCase):
-
-    def test_build_and_install(self):
-        """ test cmd `python setup.py build`
-
-        This tests that the build process and man generator run correctly.
-        It also can catch new subdirectories that weren't added to setup.py.
-        """
-        if not os.path.isfile('setup.py'):
-            raise TestSkipped('There is no setup.py file in current directory')
-        try:
-            import distutils.sysconfig
-            makefile_path = distutils.sysconfig.get_makefile_filename()
-            if not os.path.exists(makefile_path):
-                raise TestSkipped('You must have the python Makefile installed to run this test.'
-                                  ' Usually this can be found by installing "python-dev"')
-        except ImportError:
-            raise TestSkipped('You must have distutils installed to run this test.'
-                              ' Usually this can be found by installing "python-dev"')
-        self.log('test_build running in %s' % os.getcwd())
-        install_dir = osutils.mkdtemp()
+class TestSetup(tests.TestCase):
+
+    def setUp(self):
+        super(TestSetup, self).setUp()
         # setup.py must be run from the root source directory, but the tests
         # are not necessarily invoked from there
         self.source_dir = os.path.dirname(os.path.dirname(bzrlib.__file__))
-        try:
-            self.run_setup(['clean'])
-            # build is implied by install
-            ## self.run_setup(['build'])
-            self.run_setup(['install', '--prefix', install_dir])
-            self.run_setup(['clean'])
-        finally:
-            osutils.rmtree(install_dir)
+        self.build_dir = os.path.join(self.source_dir, 'build')
+        # Do we already have a build dir in the source hierarchy ?
+        if os.path.exists(self.build_dir):
+            # Put it aside during the test
+            tmp_dir = tempfile.mkdtemp(dir=self.source_dir)
+            self.addCleanup(os.rmdir, tmp_dir)
+            tmp_build_dir = os.path.join(tmp_dir, 'build')
+            os.rename(self.build_dir, tmp_build_dir)
+            # And restore the original after the test...
+            self.addCleanup(os.rename, tmp_build_dir, self.build_dir)
+        # Create a pristine build dir for the test
+        os.mkdir(self.build_dir)
+        self.addCleanup(self.rmtree, self.build_dir)
 
-    def run_setup(self, args):
+    def run_setup(self, args, retcode=0):
         args = [sys.executable, './setup.py', ] + args
         self.log('source base directory: %s', self.source_dir)
         self.log('args: %r', args)
@@ -78,5 +70,59 @@
                              stderr=self._log_file,
                              )
         s = p.communicate()
-        self.assertEqual(0, p.returncode,
+        self.assertEqual(retcode, p.returncode,
                          'invocation of %r failed' % args)
+
+    def rmtree(self, dir):
+        """Wrap osutils.rmtre
+
+        osutils.rmtree is lazy loaded and can't be used directly in cleanp hooks
+        """
+        osutils.rmtree(dir)
+
+
+class TestSetupInstall(TestSetup):
+
+    def test_build_and_install(self):
+        """ test cmd `python setup.py build`
+
+        This tests that the build process and man generator run correctly.
+        It also can catch new subdirectories that weren't added to setup.py.
+        """
+        self.requireFeature(tests.PyrexFeature)
+        if not os.path.isfile('setup.py'):
+            raise TestSkipped('There is no setup.py file in current directory')
+        try:
+            import distutils.sysconfig
+            makefile_path = distutils.sysconfig.get_makefile_filename()
+            if not os.path.exists(makefile_path):
+                raise TestSkipped('You must have the python Makefile installed'
+                                  ' to run this test. Usually this can be found'
+                                  ' by installing "python-dev"')
+        except ImportError:
+            raise TestSkipped('You must have distutils installed to run this'
+                              ' test. Usually this can be found by installing'
+                              ' "python-dev"')
+        self.log('test_build_and_install running in %s' % os.getcwd())
+        install_dir = osutils.mkdtemp()
+        self.addCleanup(self.rmtree, install_dir)
+
+        self.run_setup(['clean'])
+        # build is implied by install
+        ## self.run_setup(['build'])
+        self.run_setup(['install', '--prefix', install_dir])
+        self.run_setup(['clean'])
+
+
+class TestSetupBuild(TestSetup):
+
+    def test_with_pyrex(self):
+        self.requireFeature(tests.PyrexFeature)
+        self.run_setup(['build_ext'])
+
+    def test_without_pyrex(self):
+        if tests.PyrexFeature.available():
+            raise tests.TestSkipped('pyrex *is* installed')
+        self.run_setup(['build_ext'], retcode=1)
+
+

=== modified file 'setup.py'
--- setup.py	2009-06-23 07:10:03 +0000
+++ setup.py	2009-07-01 22:35:49 +0000
@@ -161,22 +161,27 @@
 from distutils import log
 from distutils.errors import CCompilerError, DistutilsPlatformError
 from distutils.extension import Extension
+
 ext_modules = []
+
 try:
     from Pyrex.Distutils import build_ext
 except ImportError:
     have_pyrex = False
-    # try to build the extension from the prior generated source.
+    # We will try to build the extension from the generated source.
+    from distutils.command.build_ext import build_ext
+else:
+    have_pyrex = True
+    from Pyrex.Compiler.Version import version as pyrex_version
+
+
+if not have_pyrex:
     print
     print ("The python package 'Pyrex' is not available."
            " If the .c files are available,")
     print ("they will be built,"
            " but modifying the .pyx files will not rebuild them.")
     print
-    from distutils.command.build_ext import build_ext
-else:
-    have_pyrex = True
-    from Pyrex.Compiler.Version import version as pyrex_version
 
 
 class build_ext_if_possible(build_ext):
@@ -221,15 +226,16 @@
 
 # Override the build_ext if we have Pyrex available
 command_classes['build_ext'] = build_ext_if_possible
-unavailable_files = []
+missing_pyrex_generated_files = []
 
 
 def add_pyrex_extension(module_name, libraries=None, extra_source=[]):
     """Add a pyrex module to build.
 
     This will use Pyrex to auto-generate the .c file if it is available.
-    Otherwise it will fall back on the .c file. If the .c file is not
-    available, it will warn, and not add anything.
+    Otherwise it will fall back on using the existing .c file. If the .c file
+    is not available, it will not add the extension and setup will abort
+    (whatever command is used).
 
     You can pass any extra options to Extension through kwargs. One example is
     'libraries = []'.
@@ -250,13 +256,14 @@
         source = [pyrex_name]
     else:
         if not os.path.isfile(c_name):
-            unavailable_files.append(c_name)
+            missing_pyrex_generated_files.append(c_name)
             return
         else:
             source = [c_name]
     source.extend(extra_source)
     ext_modules.append(Extension(module_name, source,
-        define_macros=define_macros, libraries=libraries))
+                                 define_macros=define_macros,
+                                 libraries=libraries))
 
 
 add_pyrex_extension('bzrlib._bencode_pyx')
@@ -292,12 +299,11 @@
                              ['bzrlib/_patiencediff_c.c']))
 
 
-if unavailable_files:
+if missing_pyrex_generated_files:
     print 'C extension(s) not found:'
-    print '   %s' % ('\n  '.join(unavailable_files),)
-    print 'The python versions will be used instead.'
+    print '  %s' % ('\n  '.join(missing_pyrex_generated_files),)
     print
-
+    exit(1)
 
 def get_tbzr_py2exe_info(includes, excludes, packages, console_targets,
                          gui_targets, data_files):