-
Notifications
You must be signed in to change notification settings - Fork 249
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
Change composer to install packages in worksapce root #1481
Change composer to install packages in worksapce root #1481
Conversation
Diff Coverage: The code coverage on the diff in this pull request is 33.3%. Total Coverage: This PR will decrease coverage by 0.06%. File Coverage Changes
🛟 Help
|
Diff Coverage: The code coverage on the diff in this pull request is 30.7%. Total Coverage: This PR will decrease coverage by 0.08%. File Coverage Changes
🛟 Help
|
4 new issues
|
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.
I added some specific comments, however I am a bit confused about the overall strategy, specifically the usage of workspace_root vs. temp_path. I think addressing the comments should hopefully help clarify.
However, this is important behavior and necessarily somewhat complex, so I think it would be valuable to add a thorough explanation of how this works as comments to the top of composer.rs
file
qlty-check/src/tool/php.rs
Outdated
@@ -169,7 +172,7 @@ impl Tool for PhpPackage { | |||
let mut env = HashMap::new(); | |||
env.insert( | |||
"COMPOSER_VENDOR_DIR".to_string(), | |||
path_to_native_string(PathBuf::from(format!("{}/vendor", self.directory()))), | |||
path_to_native_string(self.workspace_root.join("vendor")), |
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.
I notice that we seem to construct COMPOSER_VENDOR_DIR here and also on line 79 in composer.rs
with the same logic. Are these producing the same values?
If so, and the values need to be in sync, we should only compute the value once to avoid a bug if one location changes.
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.
Actually I think this is no longer needed anymore, since we are going to be installing packages in the project_root/vendor anyway
.
This was here before to tell the plugin the vendor path during invocation runs which was previously in the tool install.
For a lack of better way to pass workspace root to composer.rs this is a draft PR.