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

Give preference to actual package name over default one #9752

Conversation

adrian-martinez-interactiv4

This PR aims to modify \Composer\Repository\VcsRepository::preProcess logic according to the following use case.

Motivation

Satis fails to dump proper package names and versions after renaming package in main branch due to how packages names are retrieved by Composer when using vcs.

In fact, it uses package name found in main branch as package name for all package versions that are retrieved from vcs. I don't know if this is a feature but looks like a bug; I'd understand this behaviour to be intentional if avoiding to read data for each vcs reference, but that is not the case, as the rest of data is properly read and managed, only name fails to be respected properly.

Use case

Considering a sample, single branched repository, that contains a single package declared by a composer.json file. In the examples I'll use versions inside composer.json data just for clarity, but it happens the same without explicit versions and tags only.

Let's say I add an initial version to main branch, and tag it as 1.0.0:

{
    "name": "vendor/wrong-package-name",
    "version": "1.0.0",
    (...)
}

It becomes used with the wrong name in several projects, so I cannot simply remove it.

Then I decide its time to fix package name, and I proceed as follows:

{
    "name": "vendor/fixed-package-name",
    "version": "1.1.0",
    (...)
}

When packages are being read from vcs and added to pool, due to preProcess method behaviour, are added in the following way:
Package 1: Name: vendor/fixed-package-name, Version: 1.0.0 (does not exist as such)
Package 2: Name: vendor/fixed-package-name, Version: 1.1.0 (ok)

Instead of :
Package 1: Name: vendor/wrong-package-name, Version: 1.0.0 (ok)
Package 2: Name: vendor/fixed-package-name, Version: 1.1.0 (ok)

This issue gets still worse when fixed package name is not in the main branch, because aside of generating a new non-existing version for wrong package name, I'm unable to generate at least the fixed package name package dump, so it is not installable through Satis (no dump for vendor/fixed-package-name):
Package 1: Name: vendor/wrong-package-name, Version: 1.0.0 (ok)
Package 2: Name: vendor/wrong-package-name, Version: 1.1.0 (does not exist as such)

How to reproduce

I've mounted this example in a sample repository, located at https://bitbucket.org/interactiv4/composer-vcs-package-name.

You can test this behaviour as follows:

  • Clone the repository, cd into it and run Satis through Docker:
git clone git@bitbucket.org:interactiv4/composer-vcs-package-name.git && cd composer-vcs-package-name &&
composer clear-cache &&
docker run -it --rm \
  --volume "$(pwd):/build" \
  --volume "${COMPOSER_HOME:-$HOME/.composer}:/composer" \
  --volume "${HOME}/.ssh:/root/.ssh" \
  composer/satis build &&
tree web/dist/vendor/

Output will look something like this:

web/dist/vendor/
└── fixed-package-name
    ├── vendor-fixed-package-name-1.0.0-a01a14.zip
    └── vendor-fixed-package-name-1.1.0-528d8e.zip

Expected behaviour

This time we will require patched composer version from original PR branch:

git clone git@bitbucket.org:interactiv4/composer-vcs-package-name.git && cd composer-vcs-package-name &&
composer clear-cache &&
composer create-project composer/satis:dev-main --remove-vcs &&
composer --working-dir=satis -- config repositories.composer-vcs-package-name vcs https://github.com/adrian-martinez-interactiv4/composer &&
composer --working-dir=satis -- require 'composer/composer:dev-patch-1' &&
satis/bin/satis build && 
tree web/dist/vendor/

This time, output will look something like this:

web/dist/vendor/
├── fixed-package-name
│   └── vendor-fixed-package-name-1.1.0-528d8e.zip
└── wrong-package-name
    └── vendor-wrong-package-name-1.0.0-a01a14.zip

@stof
Copy link
Contributor

stof commented Mar 5, 2021

The fact that the package name is taken from the default branch was an explicit choice.

@adrian-martinez-interactiv4
Copy link
Author

Hi @stof , I don't understand what your comment means, would you mind explain it a little further? Just for knowing if I have missed something, thanks in advance.

@stof
Copy link
Contributor

stof commented Mar 5, 2021

The existing behavior is not a bug. It was an explicit choice of the Composer maintainers when implementing the feature.

@adrian-martinez-interactiv4
Copy link
Author

Understood, thanks for your quick response. I agree this was the intended behaviour when implemented, but it does not mean it should not be fixed/changed; as in PR description states, it may provides packages in versions that actually do not exist, while failing to provide packages with names and versions that actually exists.

Also, I can see this in current code, that is aligned with proposed change:

$tagPackageName = isset($data['name']) ? $data['name'] : $this->packageName;

@Seldaek Seldaek closed this in 1774718 Mar 9, 2021
@Seldaek
Copy link
Member

Seldaek commented Mar 9, 2021

Hopefully 1774718 clarifies it, but essentially if we do it your way it becomes impossible to rename a package without retagging every version, which I don't find reasonable.

@adrian-martinez-interactiv4
Copy link
Author

Hi @Seldaek , thanks for the clarification. Somehow I must have failed in explaining how serious is the underlying problem. Sorry if bothering, but I find it important, and I'd really like you to take a second look to the implications of current behaviour and consider them. I'll try to be as concise as possible.

I've got a proposal, mixing current behaviour with proposed one, so it could dump both package with main branch name (allowing to fully rewrite package names as intended), combined with the ability of providing also packages with the specified name in its composer.json vcs related reference. If you find interesting, let me know and I will create a new PR respecting existing behaviour and adding requested one.

Use Case

I've tried to simplify the use case exposed in PR description for easy comprehension, but I guess it does not reflect well our actual use case, so I'd like to show a real use case. This is the initial scenario:

Initial2

I hope it is self-descriptive enough. It is an specific example, but it is applicable to any package.

If we change package name in main branch from interactiv4/library-framework to interactiv4/module-framework, it will put us in a risky situation: since current behaviour will rename all older package versions, it will consider interactiv4/library-framework:110.1.0 as interactiv4/module-framework:110.1.0 at all effects. This, in practical terms, means that we lose the ability of pruning Satis; if we do that, on purpose or accidentally:

  • All package versions will be dumped as interactiv4/module-framework, and interactiv4/library-framework dumped packages won't exist anymore; all dependent packages, at all versions requiring interactiv4/library-framework will be now DEAD packages (which in our case may go up to several thundreds), as they won't be installable anymore due to its missing dependency cannot be served due to package name change.

  • Worst of all, those packages will be dead forever, as there is no way we can rebuild packages as they were before, due to vcs not being considered the "source of truth" of what packages names and versions should be dumped.

  • Even if assuming last version of dependent packages have been updated with updated package name (interactiv4/module-framework), this enforces everyone to upgrade to latest version of all packages just to have a set of installable packages, which most of times is not a trivial thing (upgrade may involve jumping through major versions and a lot of code to be adapted at once).

From my point of view:

  • Already dumped packages (with their names and versions) must be immutable at all effects, at all times. Already published packages cannot be modified or renamed, as another packages may depend on them.

  • All packages versions must be fully rebuildable at all times, taking vcs as the single source of truth, making no assumptions about changing its name, because of the previous point.

I'd really love to hear your thought about all this, thanks.

@Seldaek
Copy link
Member

Seldaek commented Mar 10, 2021

This sounds like it is a satis configuration issue more than it is a Composer issue. I don't know if Satis lets you keep the old package around, probably not because it's a fairly limited solution. On Packagist.org or Private Packagist you can have the same repository URL present twice, keep it with the old name once and re-add it with the new name. So all versions are available to both names, and you can migrate things slowly and smoothly.

I'd say if this is an important use case to you maybe consider having a look at Private Packagist?

@adrian-martinez-interactiv4
Copy link
Author

Hi, it's not a Satis configuration issue. Problematic/incomplete behaviour can be replicated by anyone using directly Composer files, it's not related with Satis itself. In this case, Satis:

  • Adds specific repositories by using \Composer\DependencyResolver\Pool::addRepository
  • Then it retrieves packages from it by using \Composer\DependencyResolver\Pool::whatProvides, using them directly, trusting what Composer logic returns.

Following PR description simplified example, it returns:

  • Package 1: Name: vendor/fixed-package-name, Version: 1.0.0
  • Package 2: Name: vendor/fixed-package-name, Version: 1.1.0

I'd expect that, at least, preserving original behaviour, it could be able to return also packages with original name by using actual vcs definition:

  • Package 1: Name: vendor/wrong-package-name, Version: 1.0.0 (original one, so it can be preserved)
  • Package 2: Name: vendor/fixed-package-name, Version: 1.0.0 (renames old tag with new package name, making it available)
  • Package 3: Name: vendor/fixed-package-name, Version: 1.1.0 (new existing package name)

This combined logic would be more accurate and complete (would solve all problems described at #9752 (comment)), while preserving BC and allowing to rename a package without retagging every version. If this sounds reasonable, I'll open a new PR with additional logic changes.

This would also fix the following:

This issue gets still worse when fixed package name is not in the main branch, because aside of generating a new non-existing version for wrong package name, I'm unable to generate at least the fixed package name package dump, because of these returned packages:
Package 1: Name: vendor/wrong-package-name, Version: 1.0.0 (ok)
Package 2: Name: vendor/wrong-package-name, Version: 1.1.0 (does not exist as such)
(Missing any vendor/fixed-package-name package at any version)

P.D.: Satis also allows to keep old packages around, also covers the rest of suggested Packagist/Private Packagist behaviour, but if it gets pruned, it cannot be fully rebuilt, leading to dead/not installable packages problem.

@naderman
Copy link
Member

naderman commented Mar 10, 2021

Hey!

Your proposal may fix this in the simple case of a single rename for you. But in reality packages end up getting renamed multiple times sometimes too, and then you're back to buggy behavior with your solution, imagine:

A package starts as foo/foo v1, renamed to bar/bar in v2. I can now install v1 as bar/bar or as foo/foo. Now in v3 the package gets renamed to baz/baz. v1 can no longer be installed as bar/bar, only as foo/foo or baz/baz.

So I think your proposal actually makes this worse in that it gives the initial impression that names keep working when renaming the package, but really they don't as soon as you do a second rename.

Alternatively doing what you propose for original and current name for all former names is quite complicated, as you'd have to go across all versions and sort them by date rather than version, which may not be very reliable, to identify which versions should then expose which former package names.

I think that the solution we have now has worked acceptably for many years and I don't see your proposal as much of an improvement.

@adrian-martinez-interactiv4
Copy link
Author

Hi @naderman, I think first part of your comment refers to suggested behaviour in PR changes. We agree those changes are not acceptable if you prioritize full package renaming over the rest of reasons exposed through PR description and comments. As such I agree to discard PR current proposed changes.

About the rest, after performing the three renaming/versioning steps, current behaviour would return:

  • Package 1: baz/baz, Version: 1.0.0 (foo/foo => bar/bar && bar/bar => baz/baz, or foo/foo => baz/baz if directly)
  • Package 2: baz/baz, Version: 2.0.0 (bar/bar => baz/baz)
  • Package 3: baz/baz, Version: 3.0.0 (current package name)

With the new proposal, after performing the three renaming/versioning steps, it would return the previous and the former names, as read from vcs:

  • Package 1: baz/baz, Version: 1.0.0 (foo/foo => bar/bar && bar/bar => baz/baz, or foo/foo => baz/baz if directly)
  • Package 2: baz/baz, Version: 2.0.0 (bar/bar => baz/baz)
  • Package 3: baz/baz, Version: 3.0.0 (current package name)
  • Package 4: foo/foo, Version: 1.0.0 (as created originally)
  • Package 5: bar/bar, Version: 2.0.0 (as created originally)

Regarding this:

Alternatively doing what you propose for original and current name for all former names is quite complicated, as you'd have to go across all versions and sort them by date rather than version, which may not be very reliable, to identify which versions should then expose which former package names.

This is being already handled by vcs drivers; the ordering it had, the ordering that it will have. In the case of BitbucketDriver, for example at https://github.com/composer/composer/blob/1.10/src/Composer/Repository/Vcs/BitbucketDriver.php#L288:

'sort' => '-target.date',

Traversing accross all versions is already being performed, and composer information for each target is already being read. Needed information for this change is already available and no additional IO is required for achieving this change. Also, the idea for this proposal is applying it only for tag refs, not for branches ones.

I think that the solution we have now has worked acceptably for many years and I don't see your proposal as much of an improvement.

Acceptably does not mean it cannot be improved to handle additional cases. Changes are BC, and the reasons for this proposed change and motivation of this new proposal are already widely explained in #9752 (comment) and #9752 (comment).

About changes, of course I think I can apply them for myself, but I wouldn't be here if I didn't think they could be a general improvement for everyone. I really think this makes sense, if not, I'd have used directly an own patched version that would work just for me.

@naderman
Copy link
Member

Think you missed my point about the problem with double rename. What you describe is exactly what I described as well. But imagine real life usage of this:

Day 1: Project P1 uses foo/foo v1
Day 2: foo/foo is renamed bar/bar and published as v2
Day 3: Project P2 uses bar/bar v1
Day 4: bar/bar is renamed baz/baz and published as v3
-> even with your changes P2 stops working here, P1 works because it uses the original name for v1, but P2 doesn't work because it used the intermediate name bar/bar for P2.

So current implementation would break both P1 and P2 because they both refer to names which no longer exist, and they need to be renamed. With your proposal, P1 keeps working because it uses the original name of v1, but P2 still breaks because it used the "current package name" at the time it was created.

So basically in your list what's missing to make this work is a *Package 6: bar/bar, Version: 1.0.0". And this can get arbitrarly complex if you add more renames.

So I think it's acceptable, and I agree, definitely not ideal, to force people to rename their requirements in this scenario rather than trying to fix it at all, if they cannot use a repository which allows saving multiple names. If the problem is reproducibility, then maybe change satis to allow you to define the names you want a package to have.

@adrian-martinez-interactiv4
Copy link
Author

Thanks a lot for the detailed explanation, I missed the point of that 6th package and I get the point of how complex and how many "virtual" packages should be dumped after renaming package many times. Now I understand clearly what you meant with:

Alternatively doing what you propose for original and current name for all former names is quite complicated, as you'd have to go across all versions and sort them by date rather than version, which may not be very reliable, to identify which versions should then expose which former package names.

Just for finishing, I'd like to say that I ended up here because I thought it all worked this way:

  • The vcs was be the single source of truth/dictionary.
  • Composer acted as an "indexer" of that dictionary, without performing any further considerations about modifying package name read from each vcs reference.
  • Released packages (with their names and versions) were kind of API, since other packages may depend on them, and they were immutable at all effects once tagged/published.
  • Due to its "indexer" nature, index (packages names and versions) were always rebuildable from vcs preserving that "API" packages.
  • Any kind of change that was wanted to be reflected in Composer should be done in vcs.

I can see now that full package renaming has a very high priority, what makes some of the previous points not possible to be implemented the way described, so any change in that direction that may interfere with full package renaming won't fit here.

Again, thank you very much for your patience, support and your time, I won't bother you anymore with this 😄

davidacmoreira-cx added a commit to cxsca/cx-composer-cli that referenced this pull request May 14, 2021
* Include stdout as well as stderr if git stash/diff/.. fails, fixes composer#9720

* Remove version argument from why and enforce it for why-not command, refs composer#9693

* Fix compiler on Windows (composer#9730)

* Preserve file permissions on Windows self-update (composer#9733)

Windows file operations result in different file permissions depending
if the file is copied or moved. A copy operation applies permissions
from the destination folder (or file if it already exists and does not
use inheritance), while a move operation generally preserves the source
file permissions.

Windows PHP `rename` uses MoveFileEx so if the user is running as an
admin and the destination is in a common (non-user) location, then the
permission for other users will be replaced by the admin user. Likewise
for the UAC elevation feature, which uses the cmd.exe `move` command.

This fix uses copy and delete operations on Windows, so that other users
can continue to run composer.phar

* Proxy handling docs and tweaks (composer#9735)

* Fix functional tests to use the same PHP version as PHPUnit runs with

* Add support for @php <abs path to binary from PATH>, fixes composer#9726

* Fix issue extracting archives into paths that already exist, fixes composer/installers#479

* Avoid using str_replace for dev-master replacement as that may be a valid part of a branch name, fixes composer#9739

* Fix php-proxying of binaries to avoid proxying phar files, fixes composer#9742

* Make sure that single files installed via file downloader get the executable bit set if they are a binary file, refs composer#9742

* Fix var shadowing

* Fix unclear error when a package can be found in lock but not in the remote repo, fixes composer#9750

* Update release step to use php8 as it produces slightly different output wrt white-space, fixes composer#9746

* Bump phpstan to level 3 (composer#9734)

Clean up PackageInterface/CompletePackageInterface, add missing methods, type things in solver as BasePackage, added CompleteAliasPackage, ..

* Update GitHub token pattern

GitHub is updating the format of auth tokens from `a-z0-9` to `A-Za-z0-9` ([notice](https://github.blog/changelog/2021-03-04-authentication-token-format-updates/)).
I'm not sure why `.` is allowed, but I dare not to remove it. In this PR, the token validation regex is updated to allow `A-Za-z0-9` instead of the current all lower-case `a-z` and disallowed `_`.

* Document GH token usage and also make sure we redact them in Process debug output, refs composer#9757

* Remove output "summary" from fund command. Fund does not provide this type of format. (composer#9748)

* Clarify behavior of name in VCS repo, closes composer#9752

* Add --format json to search command (composer#9747)


Co-authored-by: Jordi Boggiano <j.boggiano@seld.be>

* Attempt working around Vagrant filesystem issues, added COMPOSER_RUNTIME_ENV env var (set to vagrant), fixes composer#9627

* Prefer @phpstan- annotations as that is what we run against

* Fix phpdoc

* Make full functional test output more reliable

* Create pull_request_template.md

* Only call sapi_windows_set_ctrl_handler() for CLI requests (composer#9771)

* MaxFileSizeException should reject download job (composer#9778)

* Also attempt working around Vagrant filesystem issues when installing plugins initially, refs composer#9627

* Add source package name to debug info when enabling plugins

* Add dev mode env var for scripts run (composer#9793)

Co-authored-by: Vitali Tsyrkin <vitalit@playtika.com>

* Update github token pattern to match their latest updates

* Update changelog

* Tweak virtualbox detection and improve it by detecting vbox additions, refs composer#9627

* Update changelog

* Fixed detection of hg version when localized, fixes composer#9753

* Fix type warning on php8.1, refs composer#9770

* Support --no-dev combined with --locked in outdated/show commands, fixes composer#9788

* Improve InstalledVersions docs slightly

* Fix doctype annotations

* Change root.dev-requirement to root.dev in installed.php as the root is not required per se, and this simply tracks the dev mode at install time

* Fix tests

* Make ComposerRepository::configurePackageTransportOptions() protected. (composer#9818)

* Fix doctype annotations

* Switch to composer/metadata-minifier, fixes composer#9727

* Add new dep to the tests

* Change default preferred-install to dist, add --prefer-install=auto|dist|source to allow specifying auto (composer#9603)

Fixes composer#9546
Fixes composer#9674

* Add warning when loading plugins of type composer-installer as they are unlikely to function correctly and should be upgraded to the composer-plugin type

* Fix output listing some updates that do not really happen when updating mirrors/--lock, fixes composer#9812

* Make sure update mirrors/--lock keeps the release date of the original reference when dev versions have newer commits, refs composer#9812

* Fire POST_FILE_DOWNLOAD event for metadata fetched by ComposerRepository.

* Clean stuff up and deprecate old usages for PostFileDownloadEvent

* Add repository instance to Pre/PostFileDownloadEvent metadata

* Added link to composer.org docs on ProcessTimedOutException (composer#9796)

* ComposerRepository::asyncFetchFile() does not pass the downloaded URL to PostFileDownloadEvent (composer#9827)

* Update 00-intro.md

* Upgrade to xdebug-handler 2 (composer#9832)

This adds support for Xdebug3 modes and changes the default behaviour
from always restarting if Xdebug is loaded, to only restarting if Xdebug
is active.

Xdebug is considered active if it is loaded, and for Xdebug3, if it is
running in a mode other than `xdebug.mode=off`.

* Fix source links, fixes composer#9836

* Allow PreFileDownloadEvent to carry transport options for metadata (composer#9831)

* Use jsonc highlighting

* "composer init --autoload" - Interactive generates PSR-4 autoloader in composer.json (composer#9829)

- Generates PSR-4 autoload entry in composer.json.
- Run dump-autoload, if no dependencies are set

* Fix install step at the end of init command

* Fix install step at the end of init command

* Merge pull request from GHSA-h5h8-pc6h-jvvx

* Fix external process calls to avoid user input being able to pass extra parameters

* Tweak some fixes

* Merge pull request from GHSA-h5h8-pc6h-jvvx

* Fix external process calls to avoid user input being able to pass extra parameters

* Tweak some fixes

* Update changelog

* Update changelog

* Add basic source/dist validation

* Allow ints in source/dist reference

* Also make sure type is correct for preg_match

* BinaryInstaller: install full binaries on WSL when bin-compat=auto (composer#9855)

* Hint at a branch rename if we detect dev-master can not be found but dev-main or dev-default exists, fixes composer#9850

* Also condense dev-* versions if there are many, refs composer#9850

* Introduce a cross-platform safe version of is_readable to support UNC / wsl$ paths on Windows (composer#9861)

* Fix handling of inline-update-constraints with refs or stability flags, fixes composer#9847

* Link to GitLab documentation for auth (composer#9833)



Co-authored-by: Jordi Boggiano <j.boggiano@seld.be>

* Avoid leaving the event stack in a dirty state if an event listener throws, fixes composer#9846

* Fix invalid interface usage

* Fix type issues with root package interface

* Also handle throwable on supported php versions

* Fix EOL of text files (composer#9877)

* Fix update fork changes

* Add skipping svn downloader

* Revert skipping in svn downloader

Co-authored-by: Jordi Boggiano <j.boggiano@seld.be>
Co-authored-by: Nils Adermann <naderman@naderman.de>
Co-authored-by: John Stevenson <john-stevenson@blueyonder.co.uk>
Co-authored-by: Ayesh Karunaratne <ayesh@aye.sh>
Co-authored-by: ochorocho <jochen.roth@b13.com>
Co-authored-by: Brandon Kelly <brandon@pixelandtonic.com>
Co-authored-by: Stephan <glaubinix@users.noreply.github.com>
Co-authored-by: vitman <vitalyhome@tut.by>
Co-authored-by: Vitali Tsyrkin <vitalit@playtika.com>
Co-authored-by: Adam <adam@phenaproxima.net>
Co-authored-by: Markus Staab <markus.staab@redaxo.de>
Co-authored-by: Antoine Makdessi <amakdessi@me.com>
Co-authored-by: Andreas Scheibel <contact@camya.com>
Co-authored-by: Markus Staab <47448731+clxmstaab@users.noreply.github.com>
Co-authored-by: timrizzi <tim.rizzi@gmail.com>
Co-authored-by: Michael Voříšek <mvorisek@mvorisek.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants