-
Notifications
You must be signed in to change notification settings - Fork 667
Add lazy variable evaluation with lazy keyword
#2907
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
base: master
Are you sure you want to change the base?
Conversation
|
Cool feature. Maybe a test case where you produce a random number and see whether printing the variable remains the same ? |
|
@BoolPurist That's an excellent idea, I hadn't considered that. I have added a test that generates a timestamp and ensures that it is the same value when referenced more than once. It does only evaluate the value once. |
|
@MasterNayru Nice, your approach of the timestamp for testing if evaluated only once is even better/simpler compared to generating a random number. |
In my opinion the usage of this feature might be best put in its own chapter aka under its own MD heading within the README. There is also a grammar file where this new keyword Maybe replace with this |
|
I can certainly push a change up that does a best-guess at this. I was just reluctant to push a bunch of stuff that was wrong that some poor maintainer then needs to correct. This is my first contribution to a project of this size, and I don't have much experience in Rust as I learnt just enough to write this change. I was hoping that the change would be small enough that getting this over the line wouldn't involve me playing guessing games as to what should go where. If I see someone munge another Makefile of mine with macros and function calls, my eyeballs will roll out of my head and run away, so I just want to do whatever it takes to get the feature in as this feature not being there has meant that I couldn't introduce it at the last 3 companies that I've worked at. |
Fair enough. To me, you did good work so far. I appreciate that you followed my suggestion. About my suggestion for adding the documentation. I hope this feature will be merged. Here is my informal approval of this PR. |
Implements lazy evaluation for variable assignments using a new `lazy`
keyword. Lazy variables are not evaluated when the justfile loads, but
only when they are first accessed during recipe execution.
This is useful for expensive operations (like credential fetching or
remote API calls) that should only run when actually needed, not on
every justfile invocation.
Usage:
lazy credentials := `aws sts get-session-token`
# credentials are only evaluated if this recipe runs
deploy:
./deploy.sh {{credentials}}
Fixes casey#953
…e won't be recomputed each time
|
Sorry for letting this sit! Check out my comment here and let me know what you think #953 (comment). I'm interested in the trade-offs between the two approaches. Also, I was testing this branch, and I found an issue, this will print two different numbers for lazy foo := `echo $RANDOM`
a:
echo {{ foo }}
b:
echo {{ foo }}I think lazily evaluated assignments, when evaluated, are getting cached in an inner scope, not in the outer scope in which they were declared. |
Lazy variables were being re-evaluated for each recipe because each recipe gets its own child scope, and the evaluated value was only cached in that scope. A shared cache keyed by (module_path, variable_name) is now threaded through all recipe evaluations. The cache is consulted only after the assignment has been located, so the module context is known and variables with the same name in different modules are cached independently.
|
@casey I pushed a commit that I believe addresses the issue regarding variable scope. I don't know whether it's the most efficient way of handling a problem like this but I made sure to also include a test that has a Justfile that includes two other Justfiles imported with In terms of performance impact, the consequences of my decisions on the maintenance of the project, etc. I haven't given that much thought mostly because I'm not comfortable enough with Rust to spot a footgun like that. When I saw that the issue was marked initially as a "good first issue", I was quite surprised because the mechanics of a change like this would be quite profound. Having said that, my only concern at this stage would be around how easily someone could get an existing Justfile, turn a global setting on and go through the process of finding what things were depending only eager assignment evaluation. You mentioned it in your PR, but I do imagine you would want to have an eager keyword at some point. Whether you see the need to do that now, or whether you let that need organically surface is something you are much better placed to answer. At the end of day, I just think lazy evaluation is a vital feature for a project like this and I really want the project to be able to do this. I don't mind whether it's this PR that gets it over the line or not, I just want to be able to stop using Make. |
I have been checking #953 for many years now waiting for this feature to land and getting all frustrated and bent out of shape, and after having to write another Makefile, I have started hate writing Rust to put some simple implementation forward rather than suffer in silence.
This PR implements lazy evaluation for variable assignments using a new
lazykeyword. This is something that had been discussed as a potential way forward by someone, and as we all know if two people on GitHub give it a tick that is all the green lights we need.The main motivation for this is that over the years I have been unable to really suggest just as an alternative because I often need to get things like credentials to log into AWS services or some such, and having them execute on every invocation just gets way too expensive.
I am not sure what I need to update in terms of the documentation, grammar, etc. but I thought I would at least put this forward and let everyone throw tomatoes at it so at least I gave addressing the issue a try.
I have tried to write tests for this that, hopefully, cover all of the bases and edge cases, but my experience with the more exotic features of just is somewhere between zero and very limited.
Fixes #953