Skip to content
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

Prevent auto-unlocked path repo packages from also unlocking their transitive deps when -w/-W is used #10157

Merged

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Oct 14, 2021

Refs #10093

As @stof said:

@Seldaek maybe this forced inclusion of path packages should be done after building the whitelist with -w/-W, so that the path packages don't automatically whitelist their dependencies when this option are used (they would still whitelist their dependencies if the path packages are explicitly marked for update in the whitelist in the command)

@Seldaek Seldaek added the Bug label Oct 14, 2021
@Seldaek Seldaek added this to the 2.1 milestone Oct 14, 2021
@Seldaek Seldaek requested a review from naderman October 14, 2021 12:50
@stof
Copy link
Contributor

stof commented Oct 14, 2021

is there a way to test this ?

@Seldaek
Copy link
Member Author

Seldaek commented Oct 14, 2021

You mean to write tests for it? It's kinda a pain as it requires a path repo which we don't support well in the .test files. Could write a specific test for it though with some backing fixtures it should be doable.

@Seldaek Seldaek force-pushed the do_not_auto_unlock_transitive_path_deps branch from fcdf013 to 2d10eda Compare October 27, 2021 19:28
@Seldaek
Copy link
Member Author

Seldaek commented Oct 27, 2021

OK now with test 👍🏻

@naderman
Copy link
Member

naderman commented Oct 27, 2021

To add a second more meaningful test more meaningful can you actually make the package in the path repo have requirements which change from one version to the next and what effect it has when that requirement changes but the requirement is locked?

@Seldaek Seldaek force-pushed the do_not_auto_unlock_transitive_path_deps branch 2 times, most recently from 55aa41e to 9ae5067 Compare October 28, 2021 07:50
@Seldaek
Copy link
Member Author

Seldaek commented Oct 28, 2021

@naderman I modified the test to have more specific constraints and vary the constraints between locked and fresh path repo packages.

The mirrored/transitive2 gets fully unlocked including 1.0.0 which I think is correct as mirrored/path-pkg in locked version requires 1.* and when it gets unlocked we load the locked package still?

symlinked/transitive2 however gets only 1.0.1+ unlocked via the root/update requirement which is >=1.0.1.

The */transitive packages do not get unlocked as they are only referenced by path repo packages which now do not unlock transitive deps anymore.

@naderman
Copy link
Member

@Seldaek This helps. I was hoping for a second test which tests how this fails. So when a path repo changes its require constraints but because the transitive deps aren't updated its dependencies cannot be met and the thing errors.

@Seldaek Seldaek force-pushed the do_not_auto_unlock_transitive_path_deps branch 2 times, most recently from 587e349 to 984ac9a Compare November 8, 2021 13:24
@Seldaek
Copy link
Member Author

Seldaek commented Nov 8, 2021

@naderman the last commit adds a full installer test with pretty much the same inputs as the poolbuilder test, which already fails because the new transitive requirements are for 2.x and the locked ones are 1.x, it just wasn't shown as failing in poolbuilder because that doesn't run the solver. I hope this is what you meant.

@Seldaek Seldaek modified the milestones: 2.1, 2.2 Nov 9, 2021
@Seldaek Seldaek force-pushed the do_not_auto_unlock_transitive_path_deps branch 2 times, most recently from 42202dd to 5e620f2 Compare November 12, 2021 22:25
@Seldaek
Copy link
Member Author

Seldaek commented Nov 12, 2021

Rebased this on top of #10280, fixed some more bugs relating to my implementation here, and fixed the test ("symlinked/path-pkg-replace": "*" was still missing in the root requires for that package to be loaded at all), and now it looks good I believe.

@Seldaek Seldaek force-pushed the do_not_auto_unlock_transitive_path_deps branch from 5e620f2 to 3501964 Compare November 12, 2021 22:33
@Seldaek
Copy link
Member Author

Seldaek commented Nov 12, 2021

The diff is a mess tho with the two PRs so let's focus on merging #10280 first :)

@Seldaek Seldaek force-pushed the do_not_auto_unlock_transitive_path_deps branch from 3501964 to 5374a6e Compare November 23, 2021 15:52
@Seldaek
Copy link
Member Author

Seldaek commented Nov 23, 2021

OK the diff is back to a reviewable state. Most of the changes are actually to fix a bug in removeLoadedPackage when removing packages which were not locked. The rest is fairly minimal changes.

@Seldaek Seldaek requested review from naderman and removed request for naderman November 23, 2021 15:56
src/Composer/DependencyResolver/PoolBuilder.php Outdated Show resolved Hide resolved
if (isset($this->pathRepoUnlocked[$name])) {
foreach ($this->packages as $index => $package) {
if ($package->getName() === $name) {
$this->removeLoadedPackage($request, $repositories, $package, $index, false);
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why this shouldn't unlock alias packages if a remove is necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it's not clear to me why unlockPackage was ever called in removeLoadedPackage - as it's only ever called from unlockPackage, it makes no sense. The aliases have the same name as the aliased package, and unlockPackage works only based on the name, so unlocking the same name again is useless. Removed this entirely in the last commit.

@naderman
Copy link
Member

Tests look correct to me and seem to work. In general fine with this now, just the small notes I left you could still address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants