Opened 7 years ago

Closed 7 years ago

#16431 closed defect (fixed)

sync-build is broken

Reported by: vbraun Owned by:
Priority: major Milestone: sage-6.3
Component: build Keywords:
Cc: leif Merged in:
Authors: Volker Braun Reviewers: R. Andrew Ohana
Report Upstream: N/A Work issues:
Branch: e9ad958 (Commits) Commit: e9ad95891f2a2292f7b67bd1f3bc1fa2ceb10d3b
Dependencies: Stopgaps:

Description (last modified by vbraun)

sage -sync-build doesn't understand wildcards, so it deletes a bunch of correctly-installed sage.symbolic modules.

Really, the whole concept is broken. We should at the very least record what is being installed instead of trying to guess it in sync-build. Even better, make it part of the build process.

There is quite a lot of setup code, none of which is doctested. And, therefore, broken. As this ticket shows. I've started to move setup code into a sage_setup package that is installed with Sage and can be doctested in the usual manner.

Also:

  • Having SAGE_LIB point to SAGE_SRC is actually never useful, removed.
  • re-enable out-of-tree cythonize

Change History (21)

comment:1 Changed 7 years ago by vbraun

  • Authors set to Volker Braun
  • Component changed from PLEASE CHANGE to build
  • Description modified (diff)
  • Summary changed from Run sync-build before docbuild to sync-build is broken
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 7 years ago by vbraun

  • Description modified (diff)

comment:3 Changed 7 years ago by vbraun

  • Branch set to u/vbraun/remove_sync_build

comment:4 Changed 7 years ago by vbraun

  • Commit set to da331348d6fc29fab2c0c8840c3248249a1a58a6
  • Status changed from new to needs_review

New commits:

d1078bbintegrate sync-build into setup.py
b3679e5Having SAGE_LIB point to SAGE_SRC is actually never useful, removed
d6330c1automatically doctest sage_setup
af65bddmake use of the new SAGE_LIB
da33134remove annoying warning from sage -ba

comment:5 Changed 7 years ago by vbraun

  • Description modified (diff)

comment:6 Changed 7 years ago by mathzeta2

I'm not up to review this, but I do have two questions:

  1. Why not remove -ba-force if it is exactly the same as -ba? At least its description should be updated.
  2. In find_python_sources and installed_files_by_module the use os.chdir seems uneeded. Can we just use a different argument to os.walk?

comment:7 Changed 7 years ago by vbraun

  1. removal needs deprecation etc. Really it doesn't hurt to keep that alias around.
  2. its faster to construct the right relative path from the get-go than having to strip off a leading component.

comment:8 Changed 7 years ago by git

  • Commit changed from da331348d6fc29fab2c0c8840c3248249a1a58a6 to 7b26839e52667b04cc06dabbad61946dd9930f02

Branch pushed to git repo; I updated commit sha1. New commits:

7b26839re-enable out-of-tree cythonize

comment:9 Changed 7 years ago by vbraun

  • Description modified (diff)

comment:10 Changed 7 years ago by leif

  • Cc leif added

comment:11 Changed 7 years ago by leif

I think you can remove the sdist variable entirely from setup.py, i.e., also

if len(sys.argv) > 1 and sys.argv[1] == "sdist":
    sdist = True
else:
    sdist = False

comment:12 follow-up: Changed 7 years ago by vbraun

Yes, the setup.py should be broken up and doctested. And we need to decide whether we want to support sdist or not (in which case the Manifest.in can probably go, too). But thats for another ticket.

comment:13 in reply to: ↑ 12 Changed 7 years ago by leif

Replying to vbraun:

And we need to decide whether we want to support sdist or not (in which case the Manifest.in can probably go, too).

Is that (still) used anywhere in Sage? (I mean calling Sage's setup.py with sdist.)

comment:14 Changed 7 years ago by vbraun

Off-topic, but as far as I know we don't currently use distutils sdist anywhere.

comment:15 Changed 7 years ago by leif

Just for the record: This doesn't fix my docbuild-broken upgraded Sage version (but I didn't really expect it to either; I think it's rather some Sphinx-brokenness w.r.t. Python's multiprocessing).

comment:16 Changed 7 years ago by vbraun

No, thats fixed in #16440

comment:17 Changed 7 years ago by ohanar

  • Branch changed from u/vbraun/remove_sync_build to u/ohanar/remove_sync_build
  • Commit changed from 7b26839e52667b04cc06dabbad61946dd9930f02 to e9ad95891f2a2292f7b67bd1f3bc1fa2ceb10d3b

Mostly looks good, I haven't checked cython modules, but it doesn't seem like it really handles them at the moment (which would agree with some TODO I think I read in the documentation of some function). Are you planning on adding support for them at the moment, or on a future ticket?

I added a few minor reviewer changes.


New commits:

e9ad958small changes to initial sage_setup package

comment:18 Changed 7 years ago by vbraun

Just to clarify: Building / cleaning of Cython modules works. Only the doctest that no cython files are being cleaned after a successful build needs work. In particular, the module_list.py would need to be importable after building. Could be done with a sys.path hack, but I'd rather do it properly later on.

comment:19 Changed 7 years ago by vbraun

PS: any further functionality should go to a future ticket.

comment:20 Changed 7 years ago by ohanar

  • Reviewers set to R. Andrew Ohana
  • Status changed from needs_review to positive_review

Ah, yes. Looking at it again, seems fine. Everything works as expected.

comment:21 Changed 7 years ago by vbraun

  • Branch changed from u/ohanar/remove_sync_build to e9ad95891f2a2292f7b67bd1f3bc1fa2ceb10d3b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.