-
Notifications
You must be signed in to change notification settings - Fork 437
pkg: Make lock files targets #11775
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: main
Are you sure you want to change the base?
pkg: Make lock files targets #11775
Conversation
969adfe
to
6b148af
Compare
src/dune_rules/lock_rules.ml
Outdated
lock_dir; | ||
let path = Path.build target in | ||
Path.mkdir_p path; | ||
let t = Unix.localtime @@ Unix.gettimeofday () in |
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 don't think you can include this information here. It makes the action non deterministic unfortunately. You could make it part of the action input if you really want it, but i'd say it's best to skip it.
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 know. This is just for debugging, as I can't make the action rerun on changes in dune-project
and want to have a way to see whether the rule was run or not. The final PR will not have this anymore.
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've changed it temporarily to dump the package info as dyn representation into the file, that way it should be a bit more reproducible and allow debugging whether changes in the package definition lead to re-execution of the action. Weirdly I can't get it to regenerate the file, maybe I am dereferencing the Memo.t
in the wrong location and the dependency does not get registered?
src/dune_rules/lock_rules.ml
Outdated
;; | ||
|
||
let setup_lock_rules ctx_name ~lock_dir ~projects : Gen_rules.result = | ||
let sctx = Super_context.find_exn ctx_name in |
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 can't use super context for package management rules. See how you've used the lower level rules api in Pkg_rules
to work around this.
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.
Why can't the super context be used?
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.
Here you are generating the rule to produce a lock directory, but to initialize the super context you need to first load the lock directory. This will cause a cycle.
Note that the super context is useless for you here anyway. It is what we use to load the environment for basic rules, but tha environment cannot impact the generation of the lock directory. Otherwise, producing lock directories wouldn't be deterministic across machines.
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.
I don't really need the super context anyway, I just tried to use it to get a transitive dependency on the project. While I was looking at how opam_create.ml
creates the dependency, @emillon pointed out that this is most likely due to binding on Super_context.t Memo.t
. But I can just as well bind on Dune_project.t list Memo.t
which makes it a bit more explicit that I depend on the project definition.
However, even if I bind on it, I can't get my action to re-run when the dune-project
is changed, so there's still something missing I believe.
src/dune_rules/lock_rules.ml
Outdated
/ "content" | ||
in | ||
let gen_rules lock_dir = | ||
let lock_rule = lock ~target ~lock_dir in |
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 don't really get this lock_dir
argument. It's part of the path in the directory where we are loading the rule and we are passing it to the action?
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.
It's the identifier for the lock directory we want to generate. Basically the value that lock_dir
->path
from dune-workspace
. In most projects that will always just be dune.lock
, but for simplicity I thought it would be nice if the rules would generate them in an unchanging place no matter whether you have one or multiple lock directories.
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.
Okay, I understand. Although note that the path in the workspace is not necessarily a path in the root. It might be something like foo/bar/dune.lock
as well for example. So you will either need to escape it somehow or make a much more complex loading scheme.
let alias = Alias.make ~dir Alias0.pkg_lock in | ||
let rule = | ||
Rules.collect_unit (fun () -> | ||
(* careful, need to point to a file that will be created by the rule *) |
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 don't think so, you can point to the directory itself.
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 would have thought so too, but when I remove the last path element I get:
Error: No rule found for default/.lock/dune.lock (context _private)
-> required by alias dune.lock/pkg-lock
Which is surprising to me, since I believe I am attaching a rule there:
| [ ".lock"; lock_dir ] -> Memo.return @@ setup_lock_rules ~dir ~lock_dir ~projects
|
||
let setup_rules ~components ~dir ~projects ctx_name = | ||
match components with | ||
| [ ".lock" ] -> |
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 don't fully understand the scheme of rules you're creating, but I don't see why you need more than one directory to define all of your rules. I would expect the following:
- If you have a single lock directory, then you just need one rule inside
".lock"
- If you multiple lock directories, they can all co-exist in
.lock
under different names.
You don't even need a directory per context. Just a single directory for all contexts will do.
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 scheme is .lock/<path-of-the-lockdir>/<contents-here>
. I wanted to keep it uniform, so it will always be the same, no matter if the user has one or multiple lock dirs configured in their workspace. That means that <path-of-the-lockdir>
will mostly be dune.lock
but I don't see an issue with that.
That way I also hope that the promote rules that promote the generated contents back into the users source tree will be simpler to write and understand as one does not need to know whether the workspace has one or multiple lock directories configured.
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 can do it like that as well. In this particular problem, the names of the targets aren't important and should be invisible to the user, so just about any scheme can be chosen. In fact, a clever approach would pick a name that is a has of the all the inputs to the solver. That way, multiple lock directories could be represented with the same target.
I don't necessarily recommend this, just pointing out that the names are completely irrelevant.
Therefore, I suggest just picking the scheme that will be the easiest to implement. If there's other layouts you'd like to experiment with, you can always do it later. Keep in mind that in an ideal implementation, none of these file targets would even be necessary and we'd just do everything in memory and internal database files. So I really don't see the need to insist on some particular naming scheme.
Signed-off-by: Marek Kubica <[email protected]>
1c1a23c
to
7c24777
Compare
At this moment this PR is in the early stages of getting things to link up and doesn't actually lock.
Too early to review yet, just pushed to share the code publicly.