-
-
Notifications
You must be signed in to change notification settings - Fork 516
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: Fix test errors involving optional packages coxeter3, ... #36016
Conversation
…ized distribution, before testing
Documentation preview for this PR (built with commit b194d33; changes) is ready! 🎉 |
id: build | ||
if: always() && (steps.incremental.outcome == 'success' || steps.clean.outcome == 'success') | ||
run: | | ||
make build | ||
working-directory: ./worktree-image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You just split make build
from the Clean
step. How does this make it sure that the "corresponding extension modules" are installed? Prior to that, what do you mean by "corresponding extension modules"? Sorry for naive questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Clean" step is only run when something went wrong in the incremental build.
By splitting out the "make build" step from, this step gets run unconditionally.
In an editable build (default), the Sage library is monolithic: "make sagelib" is responsible for installing all of the Sage library, including the extension modules (Cython modules) that depend on installed optional packages. For example, sage.libs.coxeter3.coxeter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
env: | ||
MAKE: make -j2 | ||
SAGE_NUM_THREADS: 2 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this step be placed before the step "Build and test modularized distributions"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worried that modularized distributions made in the step "Build and test modularized distributions" could be wiped out by the "Clean" step. But they would not. The "Clean" step only cleans up the sage library. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is irrelevant. Never mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Now it looks good to me.
Thanks! |
These errors have started to show up after #35661 was merged.
Optional packages such as
coxeter3
are installed as a side effect of testing distributions such as sagemath-coxeter.Hence the doctests marked
# optional - coxeter3
are activated.Here we ensure that the corresponding extension modules are also installed in the venv where the doctests run.
📝 Checklist
⌛ Dependencies