Opened 5 years ago
Closed 2 years ago
#15423 closed enhancement (fixed)
Add autorebasing mechanism for Cygwin
Reported by:  jpflori  Owned by:  embray 

Priority:  blocker  Milestone:  sage8.0 
Component:  porting: Cygwin  Keywords:  
Cc:  kcrisman, dimpase, jdemeyer, jpflori, tscrim  Merged in:  
Authors:  Erik Bray  Reviewers:  JeanPierre Flori, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  5e494ad (Commits)  Commit:  5e494ad504d2f86b56c1c956518ad7d48807f51c 
Dependencies:  #20986  Stopgaps: 
Description (last modified by )
As explained in the old description, it is often necessary when building Sage on Cygwin to rebase its DLLs. This is the process of updating the default base addresses in memory at which a list of DLLs are loaded, such that none of those DLLs overlap each other in memory (in the normal case that they do overlap, Windows relocates them, but this frequently leads to fork errors in Cygwin).
This ticket solves the problem by running the sagerebase.sh
script as part of the sagespkg
script at the end of every package installation. It also runs sagerebase.sh
after make sagelib
for the same reason. Although running it after installing each package is generally unnecessary, sometimes it is necessary. For example, after installing Python, it's necessary to rebase to ensure that Python works, as it will be used later in the build process. Since rebasing doesn't actually add much overhead in terms of time, it's fine to just do it always (on Cygwin).
Note that this also uses sagerebase.sh
versus sagerebaseall.sh
. The difference is that the former only updates DLLs in Sage itself (i.e. under $SAGE_LOCAL
) whereas the latter also rebases all DLLs in the Cygwin installation. Because of this, the latter cannot be run from inside Cygwinall Cygwin processes must be stopped first. Obviously this would be too disruptive to the normal Sage build process, so we don't do that. It's also unnecessary. As long as rebaseall
is run once after installing Cygwin and all build dependencies, it will build a database of known DLLs and their base addresses and sizes, which can be referenced when rebasing new DLLs. Since building Sage doesn't affect any of the Cygwin system DLLs there's no need to rerun rebaseall
during a build of Sage.
Fixing this is necessary for unattended Sage builds to succeedotherwise the build will fail several times and require manual rebasing before proceeding.
Old Description
With Sage 5.12 (plus a few updated spkg, all of them available on trac), building Sage on Cygwin(32) is no harder than on Linux except for the need of rebasing a few times during the build process. This could be automated in the following way
 try to build
 if that fails and we're on Cygwin,
 grep the failing logs for error messages cuased by fork issues
 if all erros are caused by fork issues:
 rebase (using the oblivious option which can be run from bash, I think the script we included some time ago are ok),
 go back to step one
 if not fails as usual
Having this would open the way to a buildbot or a patchbot for Cygwin.
Change History (19)
comment:1 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:2 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:3 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:4 Changed 2 years ago by
 Milestone changed from sage6.4 to sage8.0
 Owner changed from (none) to embray
comment:5 Changed 2 years ago by
 Priority changed from major to blocker
comment:6 Changed 2 years ago by
 Branch set to u/embray/cygwin/ticket15423
 Commit set to 48ff8d8332131eb3af85e386ba38f9b2c78357e2
 Description modified (diff)
 Status changed from new to needs_review
New method for rebasing DLLs continuously during Sage build, which should solve this issue. I've updated the ticket description to explain exactly what this does and why, but I left the old description just for comparison.
New commits:
fcccd28  Include ECL .fas binaries (which are DLLs) when calling rebase(all)

6d5ad32  These scripts don't necessarily need to know where SAGE_ROOT is, just SAGE_LOCAL

b5daf80  Update these batch scripts so that it's not necessary to manually specify the paths of SAGE_LOCAL or CYGWIN_ROOT

23a5a04  Accept the path to SAGE_LOCAL as an argument (this is useful for running outside the Sage shell).

e2f0b40  Enhance sagerebase.sh to be able to pass additional arguments to rebase, and to wrap the call to rebaseall

df53e2f  This should be passing the all flag to sagerebase.sh

f39f086  Run sagerebase between each spkg install on Cygwin

cae4bc7  Also rebase after building sagelib itself

48ff8d8  Make the rebase call after each package quiet.

comment:7 Changed 2 years ago by
 Cc jpflori tscrim added
 Dependencies set to #20986
comment:8 Changed 2 years ago by
 Commit changed from 48ff8d8332131eb3af85e386ba38f9b2c78357e2 to e6297c39ecd374bb4c266f9cfc657e356b058db4
Branch pushed to git repo; I updated commit sha1. New commits:
e6297c3  Move the sagerebase.sh after sagelib installation to the Makefile for sagelib itself, rather than build/make/deps

comment:9 Changed 2 years ago by
 Status changed from needs_review to needs_work
Looks like that last commit has a merge conflict.
comment:10 Changed 2 years ago by
 Commit changed from e6297c39ecd374bb4c266f9cfc657e356b058db4 to ff8e807cc09c483cc84527bb06dc4464892c1a1e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
42aeeae  Include ECL .fas binaries (which are DLLs) when calling rebase(all)

b60823b  These scripts don't necessarily need to know where SAGE_ROOT is, just SAGE_LOCAL

e2905b1  Update these batch scripts so that it's not necessary to manually specify the paths of SAGE_LOCAL or CYGWIN_ROOT

5d8910c  Accept the path to SAGE_LOCAL as an argument (this is useful for running outside the Sage shell).

43eb7bf  Enhance sagerebase.sh to be able to pass additional arguments to rebase, and to wrap the call to rebaseall

784c0e2  This should be passing the all flag to sagerebase.sh

747e201  Run sagerebase between each spkg install on Cygwin

6579d0a  Also rebase after building sagelib itself

8c2d48e  Make the rebase call after each package quiet.

ff8e807  Move the sagerebase.sh after sagelib installation to the Makefile for sagelib itself, rather than build/make/deps

comment:11 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 2 years ago by
 Reviewers set to JeanPierre Flori
 Status changed from needs_review to positive_review
LGTM
comment:13 Changed 2 years ago by
 Status changed from positive_review to needs_info
In src/Makefile.in
, is this really needed for the rebase script to work?
(cd $(srcdir) \ && export SAGE_ROOT=/doesnotexist \ SAGE_SRC=/doesnotexist \ SAGE_SRC_ROOT=/doesnotexist \ SAGE_DOC_SRC=/doesnotexist \ SAGE_BUILD_DIR=/doesnotexist \ SAGE_PKGS=$(abs_top_srcdir)/build/pkgs \ SAGE_CYTHONIZED=$(abs_builddir)/build/cythonized \
If not, it would be cleaner to write the rebase script on a new line instead of continuing with &&
.
Minor comment which you might as well fix: you don't need the parentheses (cd ... && ...)
. Those just create an extra subprocess for no reason.
comment:14 Changed 2 years ago by
Nah the rebase script does only care about SAGE_LOCAL.
comment:15 Changed 2 years ago by
 Branch changed from u/embray/cygwin/ticket15423 to u/jdemeyer/cygwin/ticket15423
comment:16 Changed 2 years ago by
 Commit changed from ff8e807cc09c483cc84527bb06dc4464892c1a1e to 5e494ad504d2f86b56c1c956518ad7d48807f51c
 Reviewers changed from JeanPierre Flori to JeanPierre Flori, Jeroen Demeyer
 Status changed from needs_info to needs_review
New commits:
5e494ad  Reformat Makefile rule

comment:17 Changed 2 years ago by
 Status changed from needs_review to positive_review
Ok, let's go with jeroen solution.
comment:18 Changed 2 years ago by
You're right, this is fine.
comment:19 Changed 2 years ago by
 Branch changed from u/jdemeyer/cygwin/ticket15423 to 5e494ad504d2f86b56c1c956518ad7d48807f51c
 Resolution set to fixed
 Status changed from positive_review to closed
I've found it simpler (and I have a patch) to just always rebase after every package build. It doesn't actually add much noticeable overhead. One thing I might want to do before pushing my patch is also make it so the
quiet
flag can be passed through to rebase, since otherwise this sometimes generates a bit of noise that isn't helpful.I would say this is almost necessary for Cygwin builds; especially in an automated context like running the patchbot.