Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI Build & Test: First build incrementally and test changed files only #35652

Merged
merged 28 commits into from
Jun 21, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
4451886
.github/workflows/build.yml: First build really incrementally and tes…
mkoeppe May 19, 2023
2881586
src/sage/doctest/control.py: Make 'sage -t --new' work in git worktrees
mkoeppe May 19, 2023
43fcf4a
.github/workflows/build.yml: Run non-incremental tests with --long
mkoeppe May 19, 2023
f9b4412
.github/workflows/build.yml: Explicitly run bootstrap before the incr…
mkoeppe May 26, 2023
f93a4d6
.github/workflows/build.yml: Explicitly run bootstrap before the incr…
mkoeppe May 27, 2023
f46d247
.github/workflows/build.yml: Add comments
mkoeppe May 28, 2023
cbc0198
.github/workflows/build.yml: Add comments (fixup)
mkoeppe May 28, 2023
b8b235c
Merge remote-tracking branch 'upstream/develop' into ci_more_incremental
mkoeppe Jun 4, 2023
f4a6ca0
.github/workflows/doc-build.yml: Build incrementally
mkoeppe Jun 5, 2023
29a3ae0
build/pkgs/maxima/spkg-install.in: Patch out self-race
mkoeppe Jun 5, 2023
7d9af59
Fix up git acrobatics
mkoeppe Jun 5, 2023
a0e18d9
build/pkgs/info/spkg-configure.m4: Remove info file created in test
mkoeppe Jun 5, 2023
36b9484
.github/workflows/doc-publish.yml: Show link to CHANGES.html
mkoeppe Jun 5, 2023
5d13d8f
.github/workflows/doc-build.yml: Normalize Sage version in output bef…
mkoeppe Jun 6, 2023
2384f1d
.github/workflows/doc-build.yml: Fix path
mkoeppe Jun 6, 2023
f40fa16
.github/workflows/doc-build.yml: Ignore some generated files for diff
mkoeppe Jun 6, 2023
c865882
sage -t --new: Handle '# sage.doctest: optional' directives
mkoeppe Jun 6, 2023
21c5240
build/pkgs/{ecl,maxima}: Make info an order-only dependency
mkoeppe Jun 6, 2023
eb0ef34
.github/workflows/doc-build.yml: Fall back to non-incremental build&d…
mkoeppe Jun 6, 2023
a423367
.github/workflows/build.yml: Also do the main test in worktree-image
mkoeppe Jun 6, 2023
506db04
.github/workflows/build.yml: Remove an obsolete step
mkoeppe Jun 7, 2023
9f30a8d
.github/workflows/doc-build.yml: Include diff in zip
mkoeppe Jun 7, 2023
b13cbf4
.github/workflows/doc-build.yml: Reduce verbosity
mkoeppe Jun 7, 2023
136337c
.github/workflows/doc-build.yml: Remove redundant bootstrap
mkoeppe Jun 7, 2023
a29d22e
Add note on a doctest that randomly behaves
kwankyu Jun 12, 2023
004247c
Merge branch 'fix-doctest' into ci_more_incremental
mkoeppe Jun 12, 2023
2e5d4ce
.github/workflows/doc-build.yml: Build docs non-incrementally
mkoeppe Jun 12, 2023
908ad71
.github/workflows/build.yml: Reword test steps
mkoeppe Jun 12, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 42 additions & 14 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,52 @@ jobs:
id: checkout
uses: actions/checkout@v3

- name: Prepare
- name: Update system packages
id: prepare
run: |
# Install test tools.
if apt-get update && apt-get install -y git python3-venv; then
# Debian-specific temporary code:
# Installation of python3-venv can be removed as soon as a
# base image with a release including #33822 is available
:
else
export PATH="build/bin:$PATH"
eval $(sage-print-system-package-command auto update)
eval $(sage-print-system-package-command auto --spkg --yes --no-install-recommends install git)
fi
export PATH="build/bin:$PATH"
eval $(sage-print-system-package-command auto update)
eval $(sage-print-system-package-command auto --spkg --yes --no-install-recommends install git)

- name: Incremental build and test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this coming before the actual build/is this not building things twice now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, in fact, also run the main build on the incrementally updated source tree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this "incremental" build, what is build if one changes a python file, a cython file and say something in the build infrastructure (say a package update and a change in setup.py)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The git acrobatics has exactly the same effect as doing a "git checkout" to the new ref in a full build of the Sage distribution on a developer's computer: The timestamps (mtime) of unmodified files are kept the same, and the timestamps of modified files are newer than any file that was there before.
It is looking so complicated because the source tree in our Docker images is actually not a git working copy. This is why I have to turn it surgically into a working copy (without messing up the timestamps).

Running make build results in an incremental build: For spkgs, if the declared package version is changed, then (because they no longer match the installation records in {local,venv}/var/libs/sage/installed these packages are reinstalled.
For the Sage library, the normal in-tree editable build is run, which is incremental (only the modified Cython files are cythonized and the result compiled). Because we are using editable mode, nothing interesting is happening with the modified Python files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details!

But doesn't this mean that sometimes the incremental build (+ test) fails? At least locally, I sometimes have to hard-reset everything and run the whole bootstrap + configure + make cycle. In this cases, the workflow would fail, right? I'm not sure if this then more confusing than helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, incremental builds are very robust, with the following exceptions:

  • the docbuild process
  • after indiscriminate updates of system packages (we don't do that here on this workflow)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "hard reset", do you mean that you do "make distclean" (= remove local and venv)?

Sometimes, but most often I only have to run bootstrap & configure again.

Copy link
Contributor

@tobiasdiez tobiasdiez May 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, incremental builds are very robust, with the following exceptions:

Okay, then I let someone else with more experience on the build process have a look at this PR. Since this is the most important build workflow, it should only fail for the correct reason and not because of some general issues related to the build process.

If the incremental build is not (yet) reliable enough, I would favor a normal build + test on changed files + long test; and/or extracting the incremental build to a new workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "hard reset", do you mean that you do "make distclean" (= remove local and venv)?

Sometimes, but most often I only have to run bootstrap & configure again.

Ah OK. We can certainly run bootstrap manually before doing the incremental build. I'll make this change.
(configure definitely does not have to be run manually.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the incremental build is not (yet) reliable enough, I would favor a normal build + test on changed files + long test; and/or extracting the incremental build to a new workflow.

@tobiasdiez @kwankyu I have now implemented a fallback to non-incremental when the incremental build fails – both for build and for docbuild.

if: always() && steps.prepare.outcome == 'success'
id: incremental
run: |
set -ex
git config --global user.email "[email protected]"
git config --global user.name "Build & Test workflow"
# If actions/checkout downloaded our source tree using the GitHub REST API
# instead of with git (because do not have git installed in our image),
# we first make the source tree a repo.
if [ ! -d .git ]; then git config --global --add safe.directory $(pwd) && git init && git add -A && git commit --quiet -m "new"; fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this git acrobatics, why not get the list of changed files using say https://github.com/marketplace/actions/get-all-changed-files and then run the tests on each of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just for finding the changed files.
This trick makes the build of the Sage library actually incremental!

# Tag this state of the source tree "new". This is what we want to build and test.
git tag -f new
# Our container image contains a source tree in /sage with a full build of Sage.
# But /sage is not a git repository.
# We make /sage a worktree whose index is at tag "new".
# We then commit the current sources and set the tag "old". (This keeps all mtimes unchanged.)
# Then we update worktree and index with "git reset --hard".
# (This keeps mtimes of unchanged files unchanged and mtimes of changed files newer than unchanged files.)
# Finally we reset the index to "old". (This keeps all mtimes unchanged.)
# The changed files now show up as uncommitted changes.
git worktree add --detach worktree-image
rm -rf /sage/.git && mv worktree-image/.git /sage/
rm -rf worktree-image && ln -s /sage worktree-image
(cd worktree-image && git commit --allow-empty -m "old" -a && git tag -f old && git reset --hard new && git reset old)
# Now re-bootstrap and build. The build is incremental because we were careful with the timestamps.
# We run tests with "sage -t --new"; this only tests the uncommitted changes.
(cd worktree-image && ./bootstrap && make build && ./sage -t --new -p2)
env:
MAKE: make -j2
SAGE_NUM_THREADS: 2

- name: Configure new tree
if: always() && steps.prepare.outcome == 'success'
run: |
# Reuse built SAGE_LOCAL contained in the Docker image
./bootstrap
./configure --enable-build-as-root --prefix=/sage/local --with-sage-venv --enable-editable --enable-download-from-upstream-url

- name: Build and test modularized distributions
if: always() && steps.prepare.outcome == 'success'
run: make V=0 tox && make pypi-wheels
Expand Down Expand Up @@ -91,7 +119,7 @@ jobs:
if: always() && steps.build.outcome == 'success'
run: |
../sage -python -m pip install coverage
../sage -python -m coverage run ./bin/sage-runtests --all -p2
../sage -python -m coverage run ./bin/sage-runtests --all --long -p2
working-directory: ./src

- name: Prepare coverage results
Expand Down
2 changes: 1 addition & 1 deletion src/sage/doctest/control.py
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ def add_files(self):
# SAGE_ROOT_GIT can be None on distributions which typically
# only have the SAGE_LOCAL install tree but not SAGE_ROOT
if SAGE_ROOT_GIT is not None:
have_git = os.path.isdir(SAGE_ROOT_GIT)
have_git = os.path.exists(SAGE_ROOT_GIT)
else:
have_git = False

Expand Down