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 )
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 toSAGE_SRC
is actually never useful, removed. - re-enable out-of-tree cythonize
Change History (21)
comment:1 Changed 7 years ago by
- 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
- Description modified (diff)
comment:3 Changed 7 years ago by
- Branch set to u/vbraun/remove_sync_build
comment:4 Changed 7 years ago by
- Commit set to da331348d6fc29fab2c0c8840c3248249a1a58a6
- Status changed from new to needs_review
comment:5 Changed 7 years ago by
- Description modified (diff)
comment:6 Changed 7 years ago by
I'm not up to review this, but I do have two questions:
- Why not remove
-ba-force
if it is exactly the same as-ba
? At least its description should be updated. - In
find_python_sources
andinstalled_files_by_module
the useos.chdir
seems uneeded. Can we just use a different argument toos.walk
?
comment:7 Changed 7 years ago by
- removal needs deprecation etc. Really it doesn't hurt to keep that alias around.
- 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
- Commit changed from da331348d6fc29fab2c0c8840c3248249a1a58a6 to 7b26839e52667b04cc06dabbad61946dd9930f02
Branch pushed to git repo; I updated commit sha1. New commits:
7b26839 | re-enable out-of-tree cythonize
|
comment:9 Changed 7 years ago by
- Description modified (diff)
comment:10 Changed 7 years ago by
- Cc leif added
comment:11 Changed 7 years ago by
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: ↓ 13 Changed 7 years ago by
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
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
Off-topic, but as far as I know we don't currently use distutils sdist
anywhere.
comment:15 Changed 7 years ago by
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
No, thats fixed in #16440
comment:17 Changed 7 years ago by
- 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:
e9ad958 | small changes to initial sage_setup package
|
comment:18 Changed 7 years ago by
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
PS: any further functionality should go to a future ticket.
comment:20 Changed 7 years ago by
- 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
- Branch changed from u/ohanar/remove_sync_build to e9ad95891f2a2292f7b67bd1f3bc1fa2ceb10d3b
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
integrate sync-build into setup.py
Having SAGE_LIB point to SAGE_SRC is actually never useful, removed
automatically doctest sage_setup
make use of the new SAGE_LIB
remove annoying warning from sage -ba