-
Notifications
You must be signed in to change notification settings - Fork 599
fix: handle workspace members in needsRecompile crate collection #21284
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 all commits
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 |
|---|---|---|
|
|
@@ -73,19 +73,30 @@ async function collectCrateDirs(startCrateDir: string): Promise<string[]> { | |
| throw new Error(`Incorrectly defined dependency. Nargo.toml not found in ${absDir}`); | ||
| }); | ||
|
|
||
| // We parse and iterate over the dependencies | ||
| const parsed = TOML.parse(content) as Record<string, any>; | ||
| const deps = (parsed.dependencies as Record<string, any>) ?? {}; | ||
| for (const dep of Object.values(deps)) { | ||
| if (dep && typeof dep === 'object' && typeof dep.path === 'string') { | ||
| const depPath = resolve(absDir, dep.path); | ||
| const s = await stat(depPath); | ||
| if (!s.isDirectory()) { | ||
| throw new Error( | ||
| `Dependency path "${dep.path}" in ${tomlPath} resolves to ${depPath} which is not a directory`, | ||
| ); | ||
|
|
||
| const members = (parsed.workspace as Record<string, any>)?.members as string[] | undefined; | ||
|
|
||
| if (Array.isArray(members)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious. Is it possible for a workspace root to have both members and dependencies? If not, could we clarify that in a comment?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Workspace is a collection of crates and doesn't have its own source code. I don't think this is the place to explain what is noir workspace.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not? In this code you are assuming that the toml file either has But feel free to ignore |
||
| // The crate is a workspace root and has members defined so we visit the members | ||
| for (const member of members) { | ||
| const memberPath = resolve(absDir, member); | ||
| await visit(memberPath); | ||
| } | ||
| } else { | ||
| // The crate is not a workspace root so we check for dependencies | ||
| const deps = (parsed.dependencies as Record<string, any>) ?? {}; | ||
| for (const dep of Object.values(deps)) { | ||
| if (dep && typeof dep === 'object' && typeof dep.path === 'string') { | ||
| const depPath = resolve(absDir, dep.path); | ||
| const s = await stat(depPath); | ||
| if (!s.isDirectory()) { | ||
| throw new Error( | ||
| `Dependency path "${dep.path}" in ${tomlPath} resolves to ${depPath} which is not a directory`, | ||
| ); | ||
| } | ||
| await visit(depPath); | ||
| } | ||
| await visit(depPath); | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
Can we make it easier to read like this? And we could do the same with the other definitions above and below
Uh oh!
There was an error while loading. Please reload this page.
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.
Then the resulting
Nargo.tomlwould start with a new line. I don't think this matters much as it's already not hard to read.