Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use scheme for install rules #2130

Merged
merged 17 commits into from
May 9, 2019
Merged

Use scheme for install rules #2130

merged 17 commits into from
May 9, 2019

Conversation

aalekseyev
Copy link
Collaborator

@aalekseyev aalekseyev commented May 7, 2019 •
edited
Loading

  • Make build system explicitly ask for install rules the same way it asks for normal build rules, so that the rules can be computed on demand rather than upfront
  • Implement install rules using Scheme.t

@aalekseyev aalekseyev requested a review from a user May 7, 2019 10:10
@aalekseyev aalekseyev mentioned this pull request May 7, 2019
Merged
aalekseyev and others added 4 commits May 7, 2019 16:58
@aalekseyev
Make build system ask explicitly for install rules
d94c342 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev
Use [Scheme] for install rules
60f48c6 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@jeremiedimino @aalekseyev
Small simplifications
8b2c007 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino @aalekseyev
Memoize the package map per context
c668eb6 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@aalekseyev aalekseyev force-pushed the use-scheme-for-install-rules branch from 485ff6e to c668eb6 Compare May 7, 2019 15:58
@aalekseyev
fix
1a2eedc 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev aalekseyev mentioned this pull request May 7, 2019
Merged
@ghost
Copy link

ghost commented May 8, 2019

@aalekseyev the patch itself looks good, however looking at the final state of install_rules.ml, I feel like we could now simplify things a little. What this module does is:

  • compute a list of installation entries by scanning all the stanzas in the workspace
  • then, using this list, independently:
    • compute a map of file -> package name
    • generate the install file
    • generate symlinks from _build/<context> to _build/install/<context>

The code is not entirely written this way, however it seems like it shouldn't too hard to change it to be explicitly written this way by moving some part of install_file to init_install. What do you think of doing that, and then defining one memoized function to compute the list of all installation entries and calling this memoized function from the 3 places where we need it?

I'm also a bit worried by all the calls to Rules.file_rule. It seems to me that after the proposed refactoring, we could build a map of dir -> entries in one pass and use that to produce the symlinks. What do you think?

@rgrinberg rgrinberg mentioned this pull request May 8, 2019
Closed
aalekseyev added 2 commits May 8, 2019 11:54
@aalekseyev
combine similar install rules functions into one
befe0a7 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev
memoize install entries and use them separately
92d44cd 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev aalekseyev force-pushed the use-scheme-for-install-rules branch from e9bdf06 to 92d44cd Compare May 8, 2019 12:50
@aalekseyev
rename
5729128 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev
Copy link
Collaborator Author

I made the change along those lines, but the "generate the install file" is still not independent from "generate symlinks" because the latter rewrites the src paths used by the former.

I don't understand the concern about Rules.file_rule and what we would be improving.

@jeremiedimino
Inline get_install_entries
da7ed64 
It is called only once and it doesn't seem special compared to the
other kinds of stanzas.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
copy_installed_... --> symlink_installed_...
62be394 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link

ghost commented May 8, 2019

We discussed it offline and decided to leave the part about Rules.file_rule for later

aalekseyev and others added 3 commits May 8, 2019 18:37
@aalekseyev
Merge branch 'master' into use-scheme-for-install-rules
68edf7b
@rgrinberg
Fix indentation with ocp-indent
ad02917 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
s/fn/install_file/g
7204f11 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
rgrinberg
rgrinberg reviewed May 9, 2019
View reviewed changes
src/gen_rules.ml

String.Map.iter map ~f:(fun (module M : Gen) ->
Build_system.handle_add_rule_effects (fun () ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we port odoc to use Rules, we will not need this, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think so. This changes somewhat in #2089 and I hope will go away at some point.

rgrinberg
rgrinberg reviewed May 9, 2019
View reviewed changes
src/build_system.ml Outdated
; mutable load_dir_stack : Path.t list
; (* Set of directories under _build that have at least one rule and
all their ancestors. *)
mutable build_dirs_to_keep : Path.Set.t
; mutable prefix : (unit, unit) Build.t option
; hook : hook -> unit
; (* Package files are part of *)
packages : Package.Name.t Path.Table.t
packages : (Path.t -> Package.Name.t list) Fdecl.t
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a bit more clear if this returned Package.Name.Set.t instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this change and I'm merging the PR now.

aalekseyev added 2 commits May 9, 2019 11:17
@aalekseyev
use Package.Name.Set
59f39f5 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
@aalekseyev
simplify
f857e8f 
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
aalekseyev and others added 2 commits May 9, 2019 11:27
@aalekseyev
Merge branch 'master' into use-scheme-for-install-rules
c4e19d3
@jeremiedimino
Deforest
e3a6865 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link

ghost commented May 9, 2019

@aalekseyev I pushed one last commit to deforest a bit the construction of the map of set

@aalekseyev
Copy link
Collaborator Author

@diml thanks

@aalekseyev aalekseyev merged commit 5ed378f into ocaml:master May 9, 2019
@aalekseyev aalekseyev deleted the use-scheme-for-install-rules branch May 9, 2019 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@emillon emillon Awaiting requested review from emillon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aalekseyev @rgrinberg @jeremiedimino