Opened 4 years ago

Closed 14 months ago

#22776 closed enhancement (duplicate)

Python 2/3, exit build with error if extension modules fail to build

Reported by: embray Owned by:
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: u/embray/python-module-build-error (Commits, GitHub, GitLab) Commit: 533ff03d6057abb42516304eaf54111f9ebf90b0
Dependencies: Stopgaps:

Status badges

Description

This was motivated by, but does not solve #22768.

Currently the spkg-install for the pythons does a post-install check that some of the compiled extension modules are actually importable. The problem with this is that it's post-install, so one can still wind up with a partially broken python install.

Unfortunately, there's no good way (without patching, at least) for Python's build to exit if one of the extension modules (which are ostensibly optional) fails to build. So this patch just scans the build output for the magic string "Failed to build these modules:" and treats the build as failed if found.

This still might not be fool-proof (an extension module might somehow compile but still not be working), so this keeps the post-install checks as well. But this will at least catch more issues in build, before install.

Change History (15)

comment:1 Changed 4 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by vdelecroix

Isn't it a problem with Python build system? No upstream report?

comment:3 Changed 4 years ago by embray

I don't know that it's really a problem per se. The way it is is intentional. Though it might be nice to at least have an option to fail in this case?

comment:4 Changed 4 years ago by jhpalmieri

Do we care if some of the extension modules are broken? Wouldn't it be better to test the ones that matter to us, one by one? That is what is currently done with python2 and the _scproxy module on Darwin.

comment:5 follow-up: Changed 4 years ago by embray

There are two output sections: One that lists extension modules that were not built at all, due to some dependencies being missing or something. For now I'm assuming that's fine. This just catches extension modules that it tries to build, but has errors with (e.g. the error building the _sqlite3 module that motivated this). If it was trying to build an extension module that we explicitly didn't want and failed that would be a different issue.

comment:6 in reply to: ↑ 5 Changed 4 years ago by jhpalmieri

Replying to embray:

If it was trying to build an extension module that we explicitly didn't want and failed that would be a different issue.

This is what I was asking about. One general philosophy is to test only for the features you need and to not care if other parts are broken. You are taking a different approach: test everything, and then if someone eventually points out a feature that is broken that we don't need, then we can decide to ignore it. Please justify your choice of approach.

comment:7 Changed 4 years ago by embray

Please justify your choice of approach.

It solves the actual problem I encountered and not hypothetical ones.

comment:8 Changed 4 years ago by embray

Here's a different idea--it should be possible to move the import tests for various modules (https://git.sagemath.org/sage.git/tree/build/pkgs/python2/spkg-install?id=ee3a5d29bd4b25f964077fc6c9e8baf6bd906f61#n155) to after make is run but before make install, by using the just-built Python for the tests, and not the installed version. That should be good enough for my purposes and I think would address your concerns.

comment:9 Changed 3 years ago by jdemeyer

Since the checking has been fixed, can this be closed?

comment:10 follow-up: Changed 3 years ago by embray

Yes, I don't think this is needed now. One thing that might be good is to add more modules to the list of modules that are tested for importability after the Python build. For example, this ticket added a test to see if sqlite3 is importable which previously was not tested for.

I could either rewrite this ticket just to add sqlite3 to that list, or open a new ticket for it.

comment:11 in reply to: ↑ 10 Changed 3 years ago by jdemeyer

Replying to embray:

I could either rewrite this ticket just to add sqlite3 to that list

Fine for me.

comment:12 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:13 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-8.0 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

This is a dup of #27705 and should be closed.

comment:14 Changed 14 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:15 Changed 14 months ago by chapoton

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.