fix: remove animint2 pre-install from atime workflow (issue #340)#341
Open
ANAMASGARD wants to merge 3 commits into
Open
fix: remove animint2 pre-install from atime workflow (issue #340)#341ANAMASGARD wants to merge 3 commits into
ANAMASGARD wants to merge 3 commits into
Conversation
Pre-installing animint2 before the Autocomment action caused a namespace collision when atime installed old baseline commits: their man/*.Rd \Sexpr blocks call animint2:::rd_aesthetics, which resolved to the current package (no GeomDotplot since #311) instead of the version being built. The Autocomment action already installs HEAD, merge-base, Slow, and Fast as animint2.{sha}. Added a comment so the pre-install step is not re-added. Also trigger this workflow when atime.yaml itself changes, so the fix PR can verify the comment job passes. Fixes #340
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #341 +/- ##
==========================================
+ Coverage 73.02% 73.20% +0.18%
==========================================
Files 164 164
Lines 8837 8839 +2
==========================================
+ Hits 6453 6471 +18
+ Misses 2384 2368 -16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- animint2_atime_pkg_edit() rewrites animint2::: in cloned Slow/Fast man/*.Rd using the versioned package namespace so build-stage \Sexpr can resolve rd_aesthetics - Add tests for find_subclass() and rd_aesthetics() in test-compiler-utilities.r Fixes #340
Collaborator
|
No obvious timing issues in HEAD=fix-atime-baseline-commit-340 Generated via commit 6291631 Download link for the artifact containing the test results: ↓ atime-results.zip
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #340
The atime
commentjob has been failing on every PR that touchesR/**because the workflow pre-installed currentanimint2before the Autocomment action could install old baseline commits.Before / after
Root cause
This is not a broken baseline commit. Commit
352f7e1installs fine on its own.The problem is step order in
atime.yaml:R CMD INSTALL .loads currentanimint2into.libPaths(noGeomDotplotsince Removed geom_dotplot and replace with error #311).atime_versions_installlater tries to install the Slow baseline asanimint2.352f7e1....man/geom_dotplot.Rdhas\Sexpr{animint2:::rd_aesthetics(...)}at build time.animint2:::to the already-installed current package, not the old tree being built.Error: No geom called GeomDotplot.The Autocomment action already installs HEAD, merge-base, Slow, and Fast itself (each as
animint2.{sha}). The removed step was not just redundant — it caused the collision.What changed
R CMD INSTALL .from.github/workflows/atime.yaml..github/workflows/atime.yamlto the workflowpaths:filter so this PR triggers and verifies the fix.Slow=/Fast=in.ci/atime/tests.R— those SHAs are still correct for the Fix compiler common check speed for many polygons #238getCommonChunkcomparison.Why no testthat file
As per contributing guidelines, we do not add tests whose primary purpose is CI infrastructure. The failing
commentjob is the test; this PR makes it pass. Atestthatfile that blacklists a SHA would not catch this mechanism if someone re-added the install step.