-
Notifications
You must be signed in to change notification settings - Fork 470
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
Changes from 4 commits
99acb53
4fbf82f
c26d1ec
963d17b
f4c3dc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
lib |
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") |
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.
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 |
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") |
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.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"name": "top", | ||
"workspaces": [ | ||
"a", | ||
"b", | ||
"c" | ||
] | ||
} |
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.
Transitive things are stored in
dep_payload
, e.g. bothpackage_specs
andjsx
mode have some transitive behaviour.Not sure it's necessary to put this information in package context.
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.
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.
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 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 usingBsb_build_util.walk_all_deps
, the compiler iterates over the queue. All dependencies whose name is in that set is passed as absb_package_kind.Pinned_dependency
tobsb_ninja_regen.regenerate_ninja
. All other dependencies are passed asbsb_package_kind.Dependency
. Regardless of whether a dependency is passed as aPinned_dependency
or aDependency
, it is passed with the root project'spackage_specs
andjsx
. Any entry inpinned-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, sincebsb_ninja_regen.regenerate_ninja
needs to know whether a dependency is pinned, and I already have to calculate that inBsb_build_util.walk_all_deps
, I've modifiedBsb_build_util.walk_all_deps
to return that information as well.To address your comments specifically:
is_pinned
inpackage_context
, I chose to put it there because it makes the code simpler and reduces file reads.