-
-
Notifications
You must be signed in to change notification settings - Fork 828
Bugfix: path dependencies module resolution bug with local paths #4563
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
base: main
Are you sure you want to change the base?
Bugfix: path dependencies module resolution bug with local paths #4563
Conversation
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.
Sorry this took so long for me to review! I missed it somehow, and I've only just seen it now.
Thank you for this, I've left some comments inline. Tests will need to be added too.
compiler-cli/src/dependencies.rs
Outdated
// which indicates that their dependencies have changed | ||
for (key, requirement2) in requirements2 { | ||
if let Requirement::Path { path } = requirement2 { | ||
let dep_manifest_path = root_path.join(path).join("manifest.toml"); |
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.
All path manipulation must be done in the ProjectPaths class, no other code is permitted to use .join
etc.
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.
Done!
compiler-cli/src/dependencies.rs
Outdated
|
||
// We used to always force clean artifacts for path dependencies, | ||
// but now we use the gleam.toml hash tracking in is_same_requirements | ||
// to determine when path dependency dependencies have changed |
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 comment mean?
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.
caught my GPT generated snippet. Sorry, removing it.
compiler-cli/src/dependencies.rs
Outdated
@@ -672,6 +672,66 @@ fn is_same_requirements( | |||
} | |||
} | |||
|
|||
// For path dependencies, we need to check if their manifest.toml files have changed, | |||
// which indicates that their dependencies have changed | |||
for (key, requirement2) in requirements2 { |
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.
This equality checking logic should be in the same_requirements
function as that's the one that checks equality, while this one iterates over the collection.
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.
yeah, I removed the comment
compiler-cli/src/dependencies.rs
Outdated
} | ||
}; | ||
|
||
let mut hasher = std::hash::DefaultHasher::new(); |
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.
We use xxhash_rust::xxh3::xxh3_64
for fingerprinting, not the default hasher.
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.
Done!
tracing::debug!("cannot_read_path_dependency_manifest_forcing_rebuild"); | ||
return Ok(false); | ||
} | ||
}; |
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.
We check mtimes before we hash, to avoid extra work, so let's add that 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.
Done!
compiler-cli/src/dependencies.rs
Outdated
// If cached hash file doesn't exist, this is the first time we're checking this dependency | ||
if !cached_hash_path.exists() { | ||
// Save the current hash for future comparisons | ||
if let Err(e) = fs::write(&cached_hash_path, ¤t_hash) { |
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 errors in this code are being silently discarded! They must always be passed back up.
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.
Done!
compiler-cli/src/dependencies.rs
Outdated
|
||
if !dep_manifest_path.exists() { | ||
tracing::debug!("path_dependency_manifest_not_found_forcing_rebuild"); | ||
return Ok(false); |
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.
This would result in rebuilding every single time when there's no manifest for the path dep!
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.
Sorry! continue
-ing instead now
compiler-cli/src/dependencies.rs
Outdated
@@ -672,6 +672,66 @@ fn is_same_requirements( | |||
} | |||
} | |||
|
|||
// For path dependencies, we need to check if their manifest.toml files have changed, |
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 name for this function doesn't fit what it does any more- even if the requirements are the same the function can return false in the instance that the definition of one of the required deps has changed.
The functions have also been made effectful and to depend on file system state, so that would need to be communicated clearly in the name and arguments too.
These functions that check the requirements are the same could go unchanged (which would still fit the "is same requirements" name) and a new function could be created to check just the path deps have not changed their deps.
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.
pushed an update!
@lpil Addressed all comments! Also added tests, that you can checkout. Thank you for the input! |
Code to fix #2278
I hope this is an acceptable solution, I've inspected closed PRs that didn't make it attempting to fix this issue (#2334 and #3398).
In summary this approach stores a hash of the manifest.toml of all dependencies that are a path dependencies in the build directory and rebuilds any packages if the manifest.toml should change.
Let me know if I've missed anything or if this approach does not scale well, thank you!
Also I did not want to clutter the codebase with it, but here is a script (thank you GPT) to test the scenario that produces the error mentioned in the linked issue. It assumes you're running it in the root of the gleam codebase, otherwise you can adjust the
GLEAM_BIN
path. Also this script probably only works on a mac or linux. Here is the script: