Opened 6 years ago

Closed 3 years ago

#15423 closed enhancement (fixed)

Add auto-rebasing mechanism for Cygwin

Reported by: jpflori Owned by: embray
Priority: blocker Milestone: sage-8.0
Component: porting: Cygwin Keywords:
Cc: kcrisman, dimpase, jdemeyer, jpflori, tscrim Merged in:
Authors: Erik Bray Reviewers: Jean-Pierre Flori, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 5e494ad (Commits) Commit: 5e494ad504d2f86b56c1c956518ad7d48807f51c
Dependencies: #20986 Stopgaps:

Description (last modified by embray)

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 sage-rebase.sh script as part of the sage-spkg script at the end of every package installation. It also runs sage-rebase.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 sage-rebase.sh versus sage-rebaseall.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 Cygwin--all 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 succeed--otherwise 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 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:2 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:3 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:4 Changed 3 years ago by embray

  • Milestone changed from sage-6.4 to sage-8.0
  • Owner changed from (none) to embray

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.

comment:5 Changed 3 years ago by embray

  • Priority changed from major to blocker

comment:6 Changed 3 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/cygwin/ticket-15423
  • 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:

fcccd28Include ECL .fas binaries (which are DLLs) when calling rebase(all)
6d5ad32These scripts don't necessarily need to know where SAGE_ROOT is, just SAGE_LOCAL
b5daf80Update these batch scripts so that it's not necessary to manually specify the paths of SAGE_LOCAL or CYGWIN_ROOT
23a5a04Accept the path to SAGE_LOCAL as an argument (this is useful for running outside the Sage shell).
e2f0b40Enhance sage-rebase.sh to be able to pass additional arguments to rebase, and to wrap the call to rebaseall
df53e2fThis should be passing the --all flag to sage-rebase.sh
f39f086Run sage-rebase between each spkg install on Cygwin
cae4bc7Also rebase after building sagelib itself
48ff8d8Make the rebase call after each package quiet.

comment:7 Changed 3 years ago by embray

  • Cc jpflori tscrim added
  • Dependencies set to #20986

comment:8 Changed 3 years ago by git

  • Commit changed from 48ff8d8332131eb3af85e386ba38f9b2c78357e2 to e6297c39ecd374bb4c266f9cfc657e356b058db4

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

e6297c3Move the sage-rebase.sh after sagelib installation to the Makefile for sagelib itself, rather than build/make/deps

comment:9 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

Looks like that last commit has a merge conflict.

comment:10 Changed 3 years ago by git

  • Commit changed from e6297c39ecd374bb4c266f9cfc657e356b058db4 to ff8e807cc09c483cc84527bb06dc4464892c1a1e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

42aeeaeInclude ECL .fas binaries (which are DLLs) when calling rebase(all)
b60823bThese scripts don't necessarily need to know where SAGE_ROOT is, just SAGE_LOCAL
e2905b1Update these batch scripts so that it's not necessary to manually specify the paths of SAGE_LOCAL or CYGWIN_ROOT
5d8910cAccept the path to SAGE_LOCAL as an argument (this is useful for running outside the Sage shell).
43eb7bfEnhance sage-rebase.sh to be able to pass additional arguments to rebase, and to wrap the call to rebaseall
784c0e2This should be passing the --all flag to sage-rebase.sh
747e201Run sage-rebase between each spkg install on Cygwin
6579d0aAlso rebase after building sagelib itself
8c2d48eMake the rebase call after each package quiet.
ff8e807Move the sage-rebase.sh after sagelib installation to the Makefile for sagelib itself, rather than build/make/deps

comment:11 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:12 Changed 3 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to positive_review

LGTM

comment:13 Changed 3 years ago by jdemeyer

  • 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 3 years ago by jpflori

Nah the rebase script does only care about SAGE_LOCAL.

comment:15 Changed 3 years ago by jdemeyer

  • Branch changed from u/embray/cygwin/ticket-15423 to u/jdemeyer/cygwin/ticket-15423

comment:16 Changed 3 years ago by jdemeyer

  • Commit changed from ff8e807cc09c483cc84527bb06dc4464892c1a1e to 5e494ad504d2f86b56c1c956518ad7d48807f51c
  • Reviewers changed from Jean-Pierre Flori to Jean-Pierre Flori, Jeroen Demeyer
  • Status changed from needs_info to needs_review

New commits:

5e494adReformat Makefile rule

comment:17 Changed 3 years ago by jpflori

  • Status changed from needs_review to positive_review

Ok, let's go with jeroen solution.

comment:18 Changed 3 years ago by embray

You're right, this is fine.

comment:19 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/cygwin/ticket-15423 to 5e494ad504d2f86b56c1c956518ad7d48807f51c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.