fix(ephemerals): don't cache empty ephemerals - #2174
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2174 +/- ##
=======================================
Coverage 61.29% 61.30%
=======================================
Files 164 164
Lines 20568 20572 +4
Branches 3575 3575
=======================================
+ Hits 12607 12611 +4
Misses 7089 7089
Partials 872 872 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Ephemerals were initially exposed to package commands as an `ephemerals` binding. Since dcf77a2, they are also exposed to late-bound functions using the same strategy: reusing the existing list of resolved ephemerals. The issue lies in the difference in timing between late-bound functions and package commands: - Package commands are executed after the context have been resolved, so we get a properly resolved list of ephemerals - Late-bound functions are executed _during_ a solve, which means we don't have access to the resolved ephemerals yet The previous implementation did not crash when accessing `ephemerals` in late-bound functions: instead, it defaulted to an empty list. This behavior leads to a second issue: ephemerals were added and **cached** as part of the list of pre-resolve bindings. This can lead to the following scenario: - A package late-bound function accesses the `ephemerals` binding - Since the context is not resolve yet, no resolved ephemerals is found and an empty list is returned and cached - Later, in the package commands, we also access `ephemerals` - The cached empty list is returned, meaning that most queries will likely fail This commit addresses these issues the following way: - Move `ephemerals` out of pre-resolve bindings for commands, so that they are always retrieved from the resolved context - For late-bound functions, use resolved ephemerals only if a context is available
aa10016 to
6707d24
Compare
|
I didn't add any test yet because I'm unsure where it should be added in the test suite, and how the test package should be created. A good candidate seemed to be in the TestContext class: rez/src/rez/tests/test_context.py Line 23 in aa10016 Also, the test requires a package with specific attributes, and I'm not sure if the package should be created inline in the test or stored somewhere under |
Ephemerals were initially exposed to package commands as an
ephemeralsbinding.Since dcf77a2, they are also exposed to late-bound functions using the same strategy: reusing the existing list of resolved ephemerals.
The issue lies in the difference in timing between late-bound functions and package commands:
The previous implementation did not crash when accessing
ephemeralsin late-bound functions: instead, it defaulted to an empty list.This behavior leads to a second issue: ephemerals were added and cached as part of the list of pre-resolve bindings.
This can lead to the following scenario:
ephemeralsbindingephemeralsThis commit addresses these issues the following way:
ephemeralsout of pre-resolve bindings for commands, so that they are always retrieved from the resolved context