Allow cythonize to recompile .py file when .pxd file has changed #4063

goldenrockefeller · 2021-03-23T23:06:52Z

Adds the complementary .pxd file to the dependency tree if it exists. Fixes #1428

da-woods

Thanks.

It'd be nice to have a test for this feature. Unfortunately I don't think there's an existing test file to add it to (which is probably why it's broken right now).

Maybe a file similar to https://github.com/cython/cython/blob/474573b324fdd4e2e113043efe326971e652769b/Cython/Build/Tests/TestCyCache.py that creates a bunch of pyx, py and matching pxd files and tests that it's found the correct dependencies?

Cython/Build/Dependencies.py

Co-authored-by: da-woods <dw-git@d-woods.co.uk>

goldenrockefeller · 2021-03-24T18:51:51Z

Thanks.

It'd be nice to have a test for this feature. Unfortunately I don't think there's an existing test file to add it to (which is probably why it's broken right now).

Maybe a file similar to https://github.com/cython/cython/blob/474573b324fdd4e2e113043efe326971e652769b/Cython/Build/Tests/TestCyCache.py that creates a bunch of pyx, py and matching pxd files and tests that it's found the correct dependencies?

So like a new TestPxdDeps.py test file? Or a more general TestDependencies.py?

da-woods · 2021-03-24T18:57:54Z

If you were able to do it then the more general TestDependencies.py would be good - I'd think that doing both (py and pyx) would only be slightly longer than doing both.

I'm slightly surprised that we don't have a test for this already, but I really can't find one. (I don't know the build system hugely well though)

goldenrockefeller · 2021-03-24T19:02:30Z

Ok, I will try writing tests for dependencies.

scoder · 2021-03-24T19:19:54Z

Cython/Build/Dependencies.py

        if filename[-4:] == '.pyx' and path_exists(filename[:-4] + '.pxd'):
            pxd_list = [filename[:-4] + '.pxd']
+        elif filename[-3:] == '.py' and path_exists(filename[:-3] + '.pxd'):
+            pxd_list = [filename[:-3] + '.pxd']


This can be simplified by first using os.path.splitext().

Cython/Build/Dependencies.py

@@ -597,10 +597,16 @@ def find_pxd(self, module, filename=None):

    @cached_method
    def cimported_files(self, filename):
+<<<<<<< HEAD


scoder · 2021-03-26T19:30:10Z

Implementation looks good, test missing.

Add test to check recythonization after pxd has changed

goldenrockefeller · 2021-04-18T05:37:32Z

The dependency tree now additionally searches for dependency pxd files are relative to the dependent file path. Possibly fixing #1718

scoder · 2021-04-18T14:34:07Z

Cython/Build/Dependencies.py

@@ -534,7 +535,7 @@ def included_files(self, filename):
        for include in self.parse_dependencies(filename)[1]:
            include_path = join_path(os.path.dirname(filename), include)
            if not path_exists(include_path):
-                include_path = self.context.find_include_file(include, None)
+                include_path = self.context.find_include_file(include, (FileSourceDescriptor(filename),))


Hmm… I'd rather not create a fake code position tuple here. I'll see what I can do about it.

I boldly pushed a change to the master branch that makes this unnecessary.

scoder · 2021-04-18T14:57:24Z

Cython/Build/Tests/TestRecythonize.py

+        finally:
+            shutil.rmtree(temp_dir)


This seems a typical use case for setUp() and tearDown().

goldenrockefeller · 2021-04-26T02:02:08Z

The tests fail on Xenial, but not OS. I am not familiar with Xenial, but my guess is that I need to supply the full path direction to dependency files somewhere in the code. Is there something else I should look at?

da-woods · 2021-04-26T06:22:21Z

This is a guess, but I wonder if the timestamps that it's using to check for changes are too coarse and it isn't spotting that the file has been modified. I'd try putting a 2s pause before changing the files.

I may be wrong here, of course.

Cython/Build/Dependencies.py

scoder · 2021-04-26T12:49:58Z

Cython/Build/Tests/TestRecythonize.py

+        # The number of dependencies should be 2: "a.pxd" and "a.pyx"
+        self.assertEqual(2, len(dep_tree.all_dependencies(a_pyx)))


So, why not check for the two file names instead of just their count?

scoder · 2021-04-26T12:59:53Z

Cython/Build/Tests/TestRecythonize.py

+            a_c_contents2 = f.read()
+
+
+        self.assertNotEqual(a_c_contents1, a_c_contents2, 'C file not changed!')


Since this produces tons of uselessly unreadable output on failure, I'd rather compare them separately, (properly) print the file on failure (or, preferably, only a relevant part of it), then just assertTrue(False).

Actually, you can also assertIn("cdef int value", …) and assertIn("cdef double value", …) or something like that. Maybe both assertIn() and assertNotIn().

0dminnimda · 2021-06-21T10:28:43Z

@da-woods @scoder I found a problem due to which this PR does not pass the tests, but the author, unfortunately, is inactive, should I create my PR based on this, where would I fix all the problems?

goldenrockefeller · 2021-06-21T13:26:13Z

@da-woods @scoder I found a problem due to which this PR does not pass the tests, but the author, unfortunately, is inactive, should I create my PR based on this, where would I fix all the problems?

Is it a quick fix, I can get to it this weekend.

0dminnimda · 2021-06-21T16:40:39Z

Is it a quick fix, I can get to it this weekend.

The point is that I have already fixed it and also added more test cases.
Well, I don't know what to do in this case. Anyway, I've added enough changes for a separate PR.
@da-woods @scoder

da-woods · 2021-06-21T16:56:36Z

It sound be possible to make a PR against goldenrockfeller/cython recythonize-pxd-for-py branch. Then he'd accept it and it would become part of this PR.

I'm not sure exactly what you need to click to do it, but I've managed to do it before.

If you can get that to work, that's probably the easiest thing to do.

0dminnimda · 2021-06-21T17:14:03Z

It sound be possible to make a PR against goldenrockfeller/cython recythonize-pxd-for-py branch. Then he'd accept it and it would become part of this PR.

As far as I understood, the branch was deleted. Now it exists, it is already better.

If you can get that to work, that's probably the easiest thing to do.

I have no idea how to do this. I cannot open PR there from my fork. Also, I can't open a fork from a @goldenrockefeller fork.

…into recythonize-pxd-for-py

…kefeller/cython into recythonize-pxd-for-py

0dminnimda · 2021-06-28T04:35:07Z

Well, we are waiting for @scoder, he can run workflow.

I don't know if he will merge this PR right away, or wait until #4241 meets all the requirements.

scoder · 2021-06-28T17:58:29Z

Thanks!

goldenrockefeller added 2 commits March 23, 2021 15:50

Allow cythonize to recompile .py file when .pxd file has changed

de3318a

Fix Typo

a2699e9

da-woods reviewed Mar 24, 2021

View reviewed changes

Cython/Build/Dependencies.py Outdated Show resolved Hide resolved

Update Cython/Build/Dependencies.py

1f893a1

Co-authored-by: da-woods <dw-git@d-woods.co.uk>

scoder reviewed Mar 24, 2021

View reviewed changes

goldenrockefeller added 2 commits March 24, 2021 14:37

Simplify addition of .pxd dependency

d6f2931

Simplify addition of .pxd dependency

299b6d2

da-woods reviewed Mar 24, 2021

View reviewed changes

Cython/Build/Dependencies.py Outdated

@@ -597,10 +597,16 @@ def find_pxd(self, module, filename=None):

@cached_method

def cimported_files(self, filename):

<<<<<<< HEAD

This comment was marked as resolved.

Sign in to view

Simplify addition of .pxd dependency

cb41ba8

goldenrockefeller added 6 commits April 17, 2021 19:51

Simplify addition of .pxd dependency

3ccd5fb

Merge branch 'dependencies-tests' into recythonize-pxd-for-py

1034694

Add test to check recythonization after pxd has changed

Avoid adding extraneuos include path in recythonize tests

086663f

Remove unsused variable 'content'

56bf42d

Add tests for recythonization when a dependent pxd file changes

887d2cd

Enable relative search for dependent pxd and include files

6ee22ad

scoder reviewed Apr 18, 2021

View reviewed changes

goldenrockefeller added 2 commits April 18, 2021 14:48

Find pxd and include files directly with filename instead of using tuple

937e10a

Use tearDown command to clean up temperory test folder

b88a97b

scoder reviewed Apr 26, 2021

View reviewed changes

Fix style issues.

539f080

scoder reviewed Apr 26, 2021

View reviewed changes

0dminnimda mentioned this pull request Jun 19, 2021

Pythonise the docs #4187: "Extension types (aka. cdef classes)" (cdef_classes.rst) #4232

Merged

0dminnimda added a commit to 0dminnimda/cython that referenced this pull request Jun 21, 2021

Dependencies.py: Recreate implementation from cython#4063

d6174eb

0dminnimda added a commit to 0dminnimda/cython that referenced this pull request Jun 21, 2021

Create TestRecythonize.py based on cython#4063

4d38261

0dminnimda mentioned this pull request Jun 21, 2021

Allow cythonize to recompile .py file when .pxd file has changed, attempt 2 #4241

Open

goldenrockefeller added 6 commits June 27, 2021 22:46

Merge branch 'master' of https://github.com/goldenrockefeller/cython …

102557f

…into recythonize-pxd-for-py

Merge branch 'recythonize-pxd-for-py' of https://github.com/goldenroc…

a9bded4

…kefeller/cython into recythonize-pxd-for-py

Directly check the dependency file names

2849df4

Reduce unreadable output on failure by using assertTrue

3223382

Reduce unreadable output on failure by using assertTrue

5092564

Sleep to address coarse time-stamp precision.

0735d53

Add missing newline at end of test file.

513dae4

scoder merged commit 304b822 into cython:master Jun 28, 2021

scoder added this to the 3.0 milestone Jun 28, 2021

scoder added Build System defect labels Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow cythonize to recompile .py file when .pxd file has changed #4063

Allow cythonize to recompile .py file when .pxd file has changed #4063

goldenrockefeller commented Mar 23, 2021

da-woods left a comment

goldenrockefeller commented Mar 24, 2021

da-woods commented Mar 24, 2021

goldenrockefeller commented Mar 24, 2021

scoder Mar 24, 2021

This comment was marked as resolved.

scoder commented Mar 26, 2021

goldenrockefeller commented Apr 18, 2021

scoder Apr 18, 2021

scoder Apr 18, 2021

scoder Apr 18, 2021

goldenrockefeller commented Apr 26, 2021

da-woods commented Apr 26, 2021

scoder Apr 26, 2021

scoder Apr 26, 2021 •

edited

0dminnimda commented Jun 21, 2021 •

edited

goldenrockefeller commented Jun 21, 2021

0dminnimda commented Jun 21, 2021

da-woods commented Jun 21, 2021

0dminnimda commented Jun 21, 2021

0dminnimda commented Jun 28, 2021

scoder commented Jun 28, 2021

		# The number of dependencies should be 2: "a.pxd" and "a.pyx"
		self.assertEqual(2, len(dep_tree.all_dependencies(a_pyx)))

		a_c_contents2 = f.read()


		self.assertNotEqual(a_c_contents1, a_c_contents2, 'C file not changed!')

Allow cythonize to recompile .py file when .pxd file has changed #4063

Allow cythonize to recompile .py file when .pxd file has changed #4063

Conversation

goldenrockefeller commented Mar 23, 2021

da-woods left a comment

Choose a reason for hiding this comment

goldenrockefeller commented Mar 24, 2021

da-woods commented Mar 24, 2021

goldenrockefeller commented Mar 24, 2021

scoder Mar 24, 2021

Choose a reason for hiding this comment

This comment was marked as resolved.

scoder commented Mar 26, 2021

goldenrockefeller commented Apr 18, 2021

scoder Apr 18, 2021

Choose a reason for hiding this comment

scoder Apr 18, 2021

Choose a reason for hiding this comment

scoder Apr 18, 2021

Choose a reason for hiding this comment

goldenrockefeller commented Apr 26, 2021

da-woods commented Apr 26, 2021

scoder Apr 26, 2021

Choose a reason for hiding this comment

scoder Apr 26, 2021 • edited

Choose a reason for hiding this comment

0dminnimda commented Jun 21, 2021 • edited

goldenrockefeller commented Jun 21, 2021

0dminnimda commented Jun 21, 2021

da-woods commented Jun 21, 2021

0dminnimda commented Jun 21, 2021

0dminnimda commented Jun 28, 2021

scoder commented Jun 28, 2021

scoder Apr 26, 2021 •

edited

0dminnimda commented Jun 21, 2021 •

edited