Opened 8 years ago

Closed 8 years ago

#10233 closed defect (fixed)

Incomplete cython search path in setup.py

Reported by: vbraun Owned by: GeorgSWeber
Priority: major Milestone: sage-4.7
Component: build Keywords:
Cc: robertwb, jdemeyer Merged in: sage-4.7.alpha1
Authors: Volker Braun Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The setup.py for cython modules

  • does not record dependencies on C/C++ headers,
  • incorrectly assumes that all C/C++ headers end in '.h'.

I think both are undesirable. In particular, C++ headers often do not end in '.h', for example STL-style headers tend to do away with file extensions altogether. But setup.py relies on this:

   if not found_include:
       if path[-2:] != '.h':  # there are implicit headers from distutils, etc
           raise IOError, "could not find dependency %s included in %s."%(path, filename)

I also don't get what "implicit headers from distutil" are; The above if-clause really discards all C headers from the dependency list.

I've modified setup.py to add all C/C++ headers to the dependencies and made sure that the relevant directories are searched for them. Patch is attached.

Attachments (1)

trac_10233_fix_cython_include_path.patch (7.2 KB) - added by vbraun 8 years ago.
removed unnecessary print statement

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by vbraun

  • Cc robertwb jdemeyer added
  • Status changed from new to needs_review

I can build (sage -ba) the sage-4.6 library successfully with the patch.

comment:2 Changed 8 years ago by jdemeyer

With sage-4.6.1.alpha0 + #10094 + #10214 + this patch + several other (hopefully unrelated) patches, I get

Building modified file sage/numerical/backends/generic_backend.pyx.
Traceback (most recent call last):
  File "setup.py", line 860, in <module>
    queue = compile_command_list(ext_modules, deps)
  File "setup.py", line 820, in compile_command_list
    dep_file, dep_time = deps.newest_dep(f,m)
  File "setup.py", line 727, in newest_dep
    for f in self.all_deps(filename, ext_module):
  File "setup.py", line 710, in all_deps
    deps.update(self.all_deps(f, ext_module, path))
  File "setup.py", line 708, in all_deps
    for f in self.immediate_deps(filename, ext_module):
  File "setup.py", line 690, in immediate_deps
    self._deps[filename] = self.parse_deps(filename, ext_module)
  File "setup.py", line 680, in parse_deps
    raise IOError, "could not find dependency %s included in %s."%(path, filename)
IOError: could not find dependency float.h included in sage/numerical/backends/glpk_backend.pxd.
sage: There was an error installing modified sage library code.

comment:3 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I get the same error as above on sage-4.6.1.alpha0 with only this patch applied.

comment:4 Changed 8 years ago by vbraun

  • Status changed from needs_work to needs_review

The glpk_backend.pyx was apparently added between sage-4.6 and 4.6.1.alpha0. It uses the float.h header which gcc installs in the somewhat odd location /usr/lib/gcc/x86_64-redhat-linux/4.5.1/include. Though its perfectly legal for the C compiler to install system headers anywhere as long as it is in the default header search path.

Since there is no good way to determine the header search path except for parsing the output of cpp -v, I'll change the dependency discovery to only raise errors for *.{pyx,pxd,pxi} files. I tested it with the glpk patch #10043 applied and it builds the sage library successfully.

comment:5 Changed 8 years ago by fbissey

I am looking at this patch as part of getting your ppl work into sage-on-gentoo. One question about this part of the patch:

+include_dirs = ['%s/include'%SAGE_LOCAL,
+                '%s/include/csage'%SAGE_LOCAL,
+                '%s/sage/sage/ext'%SAGE_DEVEL,
+                '%s/sage/c_lib/include'%SAGE_DEVEL]

Why do you add the last bit: %s/sage/c_lib/include'%SAGE_DEVEL, this just duplicate the content of '%s/include/csage'%SAGE_LOCAL which should be in place already. In any case one of the two is superfluous (and make my porting work harder).

comment:6 Changed 8 years ago by vbraun

Thanks, I didn't realize that this is just a symlink. So %s/sage/c_lib/include'%SAGE_DEVEL should not be added to include_dirs.

comment:7 Changed 8 years ago by vbraun

I've updated the patch to remove the duplicated '%s/sage/c_lib/include'%SAGE_DEVEL and to get the actual gcc include directory.

comment:8 Changed 8 years ago by fbissey

Ok I will test this later along with ppl (and possibly the mpmath bump) but right now I am trying to finish off the OS X port of sage-on-gentoo (got two seriously sick packages over there: polybori and iml).

comment:9 Changed 8 years ago by mhampton

I've applied this to OS X 10.5, 10.6, and linux and had no problems after testing a variety of other tickets (including #10039, #9972, #9918). I don't feel qualified to give this a final positive review but it seems fine to me.

comment:10 Changed 8 years ago by fbissey

I am looking at this patch again. I am not sure about the last two lines in this section:

extra_include_dirs = ['%s/include/python2.6'%SAGE_LOCAL, 
                 # finally, standard C/C++ include dirs 
                 '/usr/local/include/',    
                 '/usr/include'] 

Are they really necessary? Shouldn't a standard compiler look into these by default. After all the other -I... arguments have been searched without success. Furthermore in a scenario where someone has installed their own compiler and want to use it rather the system, and have even installed there own libraries compiled with it (like a prefix basically but it doesn't have to be that complete) that means system headers may be put before their own installed headers. It is not a very nice situation which may be hard to debug.

Of course you could argue that it is not a likely scenario but it could happen. There may be more people than we think that use their own compilers rather than the system one.

comment:11 Changed 8 years ago by vbraun

Note that include_dirs / extra_include_dirs are only the directories searched for dependencies. If the dependencies are newer than the cython source, the cython module is recompiled. But these directories are not passed on to the cython compilation.

Arguably /usr/include and friends is not necessary. Listing it here only means that if you upgrade the system compiler then the cython modules will be rebuilt. Although a rare occurrence, I think that this is how it should be.

comment:12 Changed 8 years ago by fbissey

I can see the sense in that. I may have an opportunity to see how everything pans out from a gentoo prefix on OS X, since sage-on-gentoo runs there now for all intent and purposes.

comment:13 Changed 8 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

I have also tested on some platforms and I am satisfied with the patch itself after careful examination. So I will take the responsibility of giving it a positive review.

comment:14 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 8 years ago by jdemeyer

See #10751 for trouble caused by this patch. Likely, this means that I will unmerge this patch in sage-4.6.2.rc0.

comment:16 Changed 8 years ago by vbraun

#10751 is a different bug. I'm currently rewriting the setup.py and found already a couple of bugs that prevented it from picking up all dependencies. In fact, I start to wonder why it worked at all for so long. Maybe people were running sage -ba instead of complaining? I know I did a couple of times...

In any case, if you unmerge this patch then you'll just add back in one more bug.

comment:17 Changed 8 years ago by jdemeyer

In cases like this, I think the status-quo (not merging this patch) is better than merging a patch which is known to create at least one bug.

comment:18 Changed 8 years ago by jdemeyer

  • Merged in sage-4.6.2.alpha4 deleted
  • Milestone changed from sage-4.6.2 to sage-4.7
  • Resolution fixed deleted
  • Status changed from closed to new

comment:19 Changed 8 years ago by jdemeyer

  • Status changed from new to needs_work

comment:20 Changed 8 years ago by vbraun

Superseded by #10751, the patch in that ticket fixes Jeroen's issue.

comment:21 Changed 8 years ago by vbraun

  • Status changed from needs_work to positive_review

I should have said: The patch in this ticket solves the ticket, but introduces another bug with is fixed in #10751.

I'm setting this ticket back to positive review since it does fix the issue in this ticket and has been reviewed. All that is left is for somebody else to review #10751.

Changed 8 years ago by vbraun

removed unnecessary print statement

comment:22 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.