Skip to content

Made pinned dependencies transitive #5722

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

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ Usually whenever there's some issues with missing files, incompatible interfaces

```sh
./scripts/ninja.js clean # remove files not in version control
npm install
./scripts/ninja.js config
./scripts/ninja.js build
```
Expand Down
34 changes: 26 additions & 8 deletions jscomp/bsb/bsb_build_util.ml
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ let ( |? ) m (key, cb) = m |> Ext_json.test key cb

type top = Expect_none | Expect_name of string

type package_context = { proj_dir : string; top : top }
type package_context = { proj_dir : string; top : top; is_pinned: bool }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Transitive things are stored in dep_payload, e.g. both package_specs and jsx mode have some transitive behaviour.
Not sure it's necessary to put this information in package context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A perhaps more to the point comment: pinned dependencies inherit package specs from the root project, so a bool is not enough. Assuming that's all that's tracked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way that Rescript handles pinned dependencies is pretty weird. In the existing code, pinned-dependencies are just a set of strings. After finding all dependencies using Bsb_build_util.walk_all_deps, the compiler iterates over the queue. All dependencies whose name is in that set is passed as a bsb_package_kind.Pinned_dependency to bsb_ninja_regen.regenerate_ninja. All other dependencies are passed as bsb_package_kind.Dependency. Regardless of whether a dependency is passed as a Pinned_dependency or a Dependency, it is passed with the root project's package_specs and jsx. Any entry in pinned-dependencies which is not the name of a dependency is ignored.

My PR just generalizes the pre-existing behavior. Firstly I've modified Bsb_build_util.walk_all_deps so that it correctly traverses the new dependency graph. Secondly, since bsb_ninja_regen.regenerate_ninja needs to know whether a dependency is pinned, and I already have to calculate that in Bsb_build_util.walk_all_deps, I've modified Bsb_build_util.walk_all_deps to return that information as well.

To address your comments specifically:

  • While it's unnecessary to put is_pinned in package_context, I chose to put it there because it makes the code simpler and reduces file reads.
  • A bool is enough because it isn't all that's tracked because pinned dependencies are weird.


(**
TODO: check duplicate package name
Expand All @@ -146,6 +146,13 @@ type package_context = { proj_dir : string; top : top }
let pp_packages_rev ppf lst =
Ext_list.rev_iter lst (fun s -> Format.fprintf ppf "%s " s)

let extract_pinned_dependencies (map : Ext_json_types.t Map_string.t) : Set_string.t =
match Map_string.find_opt map Bsb_build_schemas.pinned_dependencies with
| None -> Set_string.empty
| Some (Arr { content }) ->
Set_string.of_list (get_list_string content)
| Some config -> Bsb_exception.config_error config "expect an array of string"

let rec walk_all_deps_aux (visited : string Hash_string.t) (paths : string list)
~(top : top) (dir : string) (queue : _ Queue.t) ~pinned_dependencies =
let bsconfig_json = dir // Literals.bsconfig_json in
Expand Down Expand Up @@ -174,9 +181,9 @@ let rec walk_all_deps_aux (visited : string Hash_string.t) (paths : string list)
if Hash_string.mem visited cur_package_name then
Bsb_log.info "@{<info>Visited before@} %s@." cur_package_name
else
let explore_deps (deps : string) =
let explore_deps (deps : string) pinned_dependencies =
map
|? ( deps,
|? ( deps,
`Arr
(fun (new_packages : Ext_json_types.t array) ->
Ext_array.iter new_packages (fun js ->
Expand All @@ -194,13 +201,24 @@ let rec walk_all_deps_aux (visited : string Hash_string.t) (paths : string list)
)
|> ignore
in
explore_deps Bsb_build_schemas.bs_dependencies;
let is_pinned = match top with
| Expect_name n when Set_string.mem pinned_dependencies n -> true
| _ -> false
in
let pinned_dependencies = match is_pinned with
| true ->
let transitive_pinned_dependencies = extract_pinned_dependencies map
in
Set_string.union transitive_pinned_dependencies pinned_dependencies
| false -> pinned_dependencies
in
explore_deps Bsb_build_schemas.bs_dependencies pinned_dependencies;
(match top with
| Expect_none -> explore_deps Bsb_build_schemas.bs_dev_dependencies
| Expect_name n when Set_string.mem pinned_dependencies n ->
explore_deps Bsb_build_schemas.bs_dev_dependencies
| Expect_none -> explore_deps Bsb_build_schemas.bs_dev_dependencies pinned_dependencies
| Expect_name _ when is_pinned ->
explore_deps Bsb_build_schemas.bs_dev_dependencies pinned_dependencies
| Expect_name _ -> ());
Queue.add { top; proj_dir = dir } queue;
Queue.add { top; proj_dir = dir; is_pinned } queue;
Hash_string.add visited cur_package_name dir
| _ -> ()

Expand Down
4 changes: 3 additions & 1 deletion jscomp/bsb/bsb_build_util.mli
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,9 @@ type result = { path : string; checked : bool }
*)
val resolve_bsb_magic_file : cwd:string -> desc:string -> string -> result

type package_context = { proj_dir : string; top : top }
type package_context = { proj_dir : string; top : top; is_pinned: bool }

val extract_pinned_dependencies: Ext_json_types.t Map_string.t -> Set_string.t

val walk_all_deps :
string -> pinned_dependencies:Set_string.t -> package_context Queue.t
11 changes: 2 additions & 9 deletions jscomp/bsb/bsb_config_parse.ml
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,6 @@ let extract_ignored_dirs (map : json_map) : Set_string.t =
Set_string.of_list (Bsb_build_util.get_list_string content)
| Some config -> Bsb_exception.config_error config "expect an array of string"

let extract_pinned_dependencies (map : json_map) : Set_string.t =
match map.?(Bsb_build_schemas.pinned_dependencies) with
| None -> Set_string.empty
| Some (Arr { content }) ->
Set_string.of_list (Bsb_build_util.get_list_string content)
| Some config -> Bsb_exception.config_error config "expect an array of string"

let extract_generators (map : json_map) =
let generators = ref Map_string.empty in
(match map.?(Bsb_build_schemas.generators) with
Expand Down Expand Up @@ -286,7 +279,7 @@ let interpret_json ~(package_kind : Bsb_package_kind.t) ~(per_proj_dir : string)
Bsb_build_schemas.bs_dev_dependencies
| Dependency _ -> []
in
let pinned_dependencies = extract_pinned_dependencies map in
let pinned_dependencies = Bsb_build_util.extract_pinned_dependencies map in
match map.?(Bsb_build_schemas.sources) with
| Some sources ->
let cut_generators =
Expand Down Expand Up @@ -357,5 +350,5 @@ let deps_from_bsconfig () =
| Obj { map } ->
( Bsb_package_specs.from_map ~cwd:Bsb_global_paths.cwd map,
Bsb_jsx.from_map map,
extract_pinned_dependencies map )
Bsb_build_util.extract_pinned_dependencies map )
| _ -> assert false
3 changes: 1 addition & 2 deletions jscomp/bsb/bsb_world.ml
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,10 @@ let make_world_deps cwd (config : Bsb_config_types.t option)
);
close_out oc ; *)
queue
|> Queue.iter (fun ({ top; proj_dir } : Bsb_build_util.package_context) ->
|> Queue.iter (fun ({ top; proj_dir; is_pinned } : Bsb_build_util.package_context) ->
match top with
| Expect_none -> ()
| Expect_name s ->
let is_pinned = Set_string.mem pinned_dependencies s in
if is_pinned then print_endline ("Dependency pinned on " ^ s)
else print_endline ("Dependency on " ^ s);
let lib_bs_dir = proj_dir // lib_artifacts_dir in
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
lib
27 changes: 27 additions & 0 deletions jscomp/build_tests/transitive_pinned_dependency1/a/bsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"name": "a",
"namespace": true,
"sources": [
{
"dir": "src"
},
{
"dir": "tests",
"type": "dev"
}
],
"package-specs": {
"module": "commonjs",
"in-source": false
},
"warnings": {
"error": true
},
"suffix": ".mjs",
"bs-dependencies": [
"b"
],
"pinned-dependencies": [
"b"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "a",
"version": "1.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Js.Console.log("src")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Js.Console.log("test")
9 changes: 9 additions & 0 deletions jscomp/build_tests/transitive_pinned_dependency1/input.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//@ts-check
var child_process = require("child_process");
var assert = require("assert");
var fs = require("fs")
var rescript_exe = require("../../../scripts/bin_path").rescript_exe;

console.log(child_process.execSync(rescript_exe, { encoding: "utf8", cwd: "./a" }));

assert(fs.existsSync("./node_modules/c/lib/js/tests/test.mjs"), "dev files of module 'c' were not built by 'a' even though 'c' is a transitive pinned dependency of 'a' through 'b'")

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions jscomp/build_tests/transitive_pinned_dependency1/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "top",
"workspaces": [
"a",
"b",
"c"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
lib
24 changes: 24 additions & 0 deletions jscomp/build_tests/transitive_pinned_dependency2/a/bsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"name": "a",
"namespace": true,
"sources": [
{
"dir": "src"
},
{
"dir": "tests",
"type": "dev"
}
],
"package-specs": {
"module": "commonjs",
"in-source": false
},
"warnings": {
"error": true
},
"suffix": ".mjs",
"bs-dependencies": [
"b"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "a",
"version": "1.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Js.Console.log("src")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Js.Console.log("test")
9 changes: 9 additions & 0 deletions jscomp/build_tests/transitive_pinned_dependency2/input.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//@ts-check
var child_process = require("child_process");
var assert = require("assert");
var fs = require("fs")
var rescript_exe = require("../../../scripts/bin_path").rescript_exe;

console.log(child_process.execSync(rescript_exe, { encoding: "utf8", cwd: "./a" }));

assert(!fs.existsSync("./node_modules/c/lib/js/tests/test.mjs"), "dev files of module 'c' were built by 'a' even though 'c' is not a pinned dependency of 'a'")

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions jscomp/build_tests/transitive_pinned_dependency2/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "top",
"workspaces": [
"a",
"b",
"c"
]
}
Loading