-
Notifications
You must be signed in to change notification settings - Fork 456
Update tutorial to use autolocking #12859
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
Update tutorial to use autolocking #12859
Conversation
|
Unfortunately I had to remove a lot of output where the solution was displayed, because with autolocking the solution is not displayed anymore. I've opened #12865 to discuss how to improve the user experience in that case, giving a users a way to figure out which packages + versions were selected by the solver. |
c435b75 to
eb5bcf5
Compare
| packages are published in the opam-repository occasionally, Dune might pick | ||
| different solutions over time. |
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 think this isn't a very robust account of the dynamics at play. Whether or not new packages are published, different builds can end up with different dependencies based on a number of factors: the system run on, the which repos are pinned, whether or not the user has had connectivity, whether packages are marked unavailable, etc. I think it's better to just describe the core need that motivates locking. And you do that in the next paragraph. So maybe this sentence can be removed?
aad05d3 to
15a0e31
Compare
Sudha247
left a comment
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.
Good to have this updated!
Some remarks below. I also found mentions of dune pkg lock in howto/use-opam-alongside-dune-package-management.rst and howto/homebrew-package.rst which we might want to check.
9e4d89d to
598ff93
Compare
|
Is this ready for another review? Or is it still being worked on? |
|
Yes, I welcome some reviews so maybe we can get it merged in time for 3.21. |
598ff93 to
eebe860
Compare
| longer than usual, as the dependencies need to be built first. Subsequent | ||
| builds where all dependencies have been built before will be just as fast as | ||
| before. | ||
| Since this is the first time we have run the build system after enabling package |
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 seems this is getting folded in the collapsible dune-workspace. I'm not sure that is intended?
Sudha247
left a comment
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 gone through the tutorial once to validate we get the expected results, and we do. I think this is good to go.
shonfeder
left a comment
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 have a few more suggestions, but they don't need to blocking.
|
|
||
| which will download and install the new dependencies and build our project as | ||
| before. | ||
| This will recongnize that the project depends on new packages. Thus it will |
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.
What does 'this' refer to here?
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.
Good question. I've clarified it and rewrote some paragraphs that also started with "This" to be more precise and hopefully less repetitive.
| (lock_dir | ||
| (repositories overlay1 upstream1)) |
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.
Ugh, that is awkward :( seems to be leaking implementation details, but that can be addressed in some followup.
d939e9d to
5925442
Compare
Signed-off-by: Marek Kubica <[email protected]>
5925442 to
ff00b58
Compare
Updates to the tutorial to use autolocking (plus explain how to manually lock).
Closes #12796