renovatebot / renovate

Home of the Renovate CLI: Cross-platform Dependency Automation by Mend.io
https://mend.io/renovate
GNU Affero General Public License v3.0
17.86k stars 2.37k forks source link

[BUG] Composer version should work with or without `v` prefix #2264

Closed swashata closed 6 years ago

swashata commented 6 years ago

What Renovate type are you using? Renovate CLI

Describe the bug

In composer, we can define version number with or without v prefix. So both 1.1.0 and v1.1.0 are valid and equivalent.

But renovateapp when manually looks for version through packagist API, it doesn't remove the v prefix and produces error.

It happens for packages where the versions are prefixed with v. For packages where versions aren't prefixed with v it works.

So to summarize this behavior gives two bugs

  1. Renovateapp does not properly update packages when there is a v prefix in published and local package version. Because I guess semver logic can not calculate with a non-numeric v within the version number.
  2. When there isn't any v prefix it works good, but for some reason, if the local composer.json has a v prefix (even if the remote does not), then renovateapp fails to detect its availability. IMO, this should be the case because we should write version according to publisher. But when the opposite happens, i.e, the remote has v prefixed version and local composer.json has non-prefixed version, then renovateapp does detect that the version is unpublished (#2250) but it fails to determine the most recent version (due to the fact that the remote has all versions prefixed with v).

To Reproduce

Case 1

When renovate app fails to update version because of v prefix.

Add in composer.json

{
    "require": {
        "drewm/mailchimp-api": "^v2.4"
    }
}

Run renovateapp.

The latest version of the package is 2.5, but I guess renovateapp doesn't detect that at all. Instead it suggests upgrading to v2.2.4 which according to string compare is greatest.

Case 2

When renovate app fails to detect a version due to inconsistency of versioning between local and remote.

Add in composer.json

{
    "require": {
        "drewm/mailchimp-api": "^2.4"
    }
}

Run renovate app.

It will report error that 2.4 is not published in the registry, which is good. But then it suggests to rollback to v2.2.4. I believe a string compare is being done here where "v2.2.4" logically becomes the greatest.

Expected behavior

Just like composer itself, it should have found that 2.4 and v2.4 are same and reported back with upgrade to v2.5 (because the remote has v2.5, not 2.5).

Additional context

When we request through packagist API, we get a response like this (for the above package)

GET https://packagist.org/packages/drewm/mailchimp-api.json
{
    "package": {
        "name": "drewm/mailchimp-api",
        "description": "Super-simple, minimum abstraction MailChimp API v3 wrapper",
        "time": "2013-12-18T14:08:09+00:00",
        "maintainers": [
            {
                "name": "drewm",
                "avatar_url": "https://www.gravatar.com/avatar/74c8cca18339c66ca10979569f1bb206?d=identicon"
            }
        ],
        "versions": {
            "v2.5": {
                "name": "drewm/mailchimp-api",
                "description": "Super-simple, minimum abstraction MailChimp API v3 wrapper",
                "keywords": [

                ],
                "homepage": "https://github.com/drewm/mailchimp-api",
                "version": "v2.5",
                "version_normalized": "2.5.0.0",
                "license": [
                    "MIT"
                ],
                "authors": [
                    {
                        "name": "Drew McLellan",
                        "email": "drew.mclellan@gmail.com",
                        "homepage": "http://allinthehead.com/"
                    }
                ],
                "source": {
                    "type": "git",
                    "url": "https://github.com/drewm/mailchimp-api.git",
                    "reference": "f532fa26cd6e7d17c9ba40a757d8c6bfee47dace"
                },
                "dist": {
                    "type": "zip",
                    "url": "https://api.github.com/repos/drewm/mailchimp-api/zipball/f532fa26cd6e7d17c9ba40a757d8c6bfee47dace",
                    "reference": "f532fa26cd6e7d17c9ba40a757d8c6bfee47dace",
                    "shasum": ""
                },
                "type": "library",
                "time": "2018-02-16T15:31:05+00:00",
                "autoload": {
                    "psr-4": {
                        "DrewM\\MailChimp\\": "src"
                    }
                },
                "require": {
                    "php": ">=5.3",
                    "ext-curl": "*"
                },
                "require-dev": {
                    "phpunit/phpunit": "7.0.*",
                    "vlucas/phpdotenv": "^2.0"
                }
            },
            "v2.4": {
                "name": "drewm/mailchimp-api",
                "description": "Super-simple, minimum abstraction MailChimp API v3 wrapper",
                "keywords": [

                ],
                "homepage": "https://github.com/drewm/mailchimp-api",
                "version": "v2.4",
                "version_normalized": "2.4.0.0",
                "license": [
                    "MIT"
                ],
                "authors": [
                    {
                        "name": "Drew McLellan",
                        "email": "drew.mclellan@gmail.com",
                        "homepage": "http://allinthehead.com/"
                    }
                ],
                "source": {
                    "type": "git",
                    "url": "https://github.com/drewm/mailchimp-api.git",
                    "reference": "fe480bb652f85270227bf6d639b0026a531f21fc"
                },
                "dist": {
                    "type": "zip",
                    "url": "https://api.github.com/repos/drewm/mailchimp-api/zipball/fe480bb652f85270227bf6d639b0026a531f21fc",
                    "reference": "fe480bb652f85270227bf6d639b0026a531f21fc",
                    "shasum": ""
                },
                "type": "library",
                "time": "2017-02-16T13:24:20+00:00",
                "autoload": {
                    "psr-4": {
                        "DrewM\\MailChimp\\": "src"
                    }
                },
                "require": {
                    "php": ">=5.3",
                    "ext-curl": "*"
                },
                "require-dev": {
                    "phpunit/phpunit": "4.0.*",
                    "vlucas/phpdotenv": "^2.0"
                }
            }
        }
    }
}

Notice that all the semvers are prefixed with v (v2.5, v2.4).

So for this to work renovateapp would need to

  1. Strip off starting v from local composer.json.
  2. Strip off starting v from remote response.
  3. Compare and do its stuff.
  4. Put back the updated version in local composer.json with or without v as found in remote response.

Since there is also a version_normalized field in the response, we can make use of that.

I have tested a few cases and I have found that, since it is quite normal to use v prefix in packagist world, many of the packages are not being updated by renovateapp, because it is not comparing the without v value.

I will do some more testing with the latest version from your pull request #2255 and will report back if I find anything more.

rarkins commented 6 years ago

If the versions on packagist uses v and composer.json does not, should we keep "stripping" the v when upgrading, or keep the user's "preference" for non-prefixed?

But what if the versions on packagist were without prefixes but then suddenly the v prefix is added to the latest version? Do we update composer.json to with v or without?

And importantly, can we realistically tell the difference? My preference is to be generous in how we compare, but strict in how we replace - i.e. always replace with the same string that's on packagist.

swashata commented 6 years ago
And importantly, can we realistically tell the difference? My preference is to be generous in how we compare, but strict in how we replace - i.e. always replace with the same string that's on packagist.

This is exactly what I was suggesting. Compare whatever is on the composer.json. But if it needs upgrade, then replace with the same string that's on packagist.

If the versions on packagist uses v and composer.json does not, should we keep "stripping" the v when upgrading, or keep the user's "preference" for non-prefixed?

In this case, it is actually user's fault to not use the v. Because, when we run composer update it appends the v if it is on packagist. So if upgrade is needed, we should add the prefixed version.

But what if the versions on packagist were without prefixes but then suddenly the v prefix is added to the latest version? Do we update composer.json to with v or without?

That almost never happens, i.e, existing versions do not change their syntax. Maybe a future version can. In any case, IMO, we shouldn't do anything if there isn't any upgrade necessary. If upgrade is necessary, then replace with what is found in packagist.

So, theoretically the steps would be as follows:

  1. Strip off starting v from local composer.json if needed.
  2. Make a request to packagist.
  3. Strip off starting v from remote response if needed.
  4. Compare local version with latest remote version.
  5. If both are same, then do nothing.
  6. If both aren't same, then replace what-ever in local composer.json with what-ever we found on packagist.

This should take care of all three scenarios.

rarkins commented 6 years ago

Thanks, that makes sense to me too

rarkins commented 6 years ago

I assume that when ranges are used in composer.json that the v is stripped even if present on packaging? Eg v1.0.0 on packagist would still be like ~1.0.0 if defined as a range?

swashata commented 6 years ago
I assume that when ranges are used in composer.json that the v is stripped even if present on packaging? Eg v1.0.0 on packagist would still be like ~1.0.0 if defined as a range?

Good catch. I've found that

  1. If you do composer require drewm/mailchimp-api:~2.0, then in composer.json there is "drewm/mailchimp-api": "~2.0".
  2. If you do composer require drewm/mailchimp-api:~v2.0, then in composer.json there is "drewm/mailchimp-api": "~v2.0".
  3. If you simply do composer require drewm/mailchimp-api, then in composer.json there is "drewm/mailchimp-api": "^2.5".

The 1 and 2, simply means that composer just puts v (or not), depending on user request. 3 is interesting, because if we let composer decide the version during first require, it doesn't add the v, although it is there in packagist.

So I guess, it really is upto you. Personally I would not not prefer a v, but that's a personal choice. So maybe we can change it to non-v-prefix, when we are updating? It won't hammer the working of composer in any way that's for sure.

If user were to manually change the range then of course, it would be preferable to put in what packagist provides (with v, in our example).

Let's bring in some folks from composer and see if they can shed some light.

/cc (and sorry) @Seldaek, @naderman, @alcohol, @igorw, @curry684

alcohol commented 6 years ago

Versions (tags) may contain a v prefix, but we generally ignore it when we normalize the version number and recommend you do not create tags with a v prefix. Version constraints that contain no v prefix should match tags that contain a v prefix.

curry684 commented 6 years ago

we generally ignore it

Composer always strips v-prefixes in every situation: https://github.com/composer/semver/blob/master/src/VersionParser.php#L129

swashata commented 6 years ago

So I guess, the right answer here is to always strip v from composer.json if upgrade is needed.

curry684 commented 6 years ago

It depends on what DX you want. To Composer the prefix is cosmetic and internally stripped. Packagist respects it - if the tags in upstream begin with a v Packagist will show them. It can harmlessly do this because it knows Composer always ignores it. It's sort of legacy with no intent to deprecate.

If developers really want to use the v in their file I don't think Renovate should be the one to decide they don't. So I'd recommend stripping it, do your operation, and reattach if it was originally there. Kinda what 'we' do in example 2 of your previous post.

Generally accepted convention for the past few years has been not to use v prefixes, I know of several libraries that have dropped them in the release schemes along the way. If you look at the big projects like PhpUnit they release without. Funnily Laravel still uses them consistently, as does Symfony.

rarkins commented 6 years ago

Thanks for the advice. If Composer doesn’t care and strips the v in either direction then it seems safe for us to try to replace with the user’s preferred notation even if it mismatches with what’s on packagist.

renovate-bot commented 6 years ago

:tada: This issue has been resolved in version 13.21.5 :tada:

The release is available on:

Your semantic-release bot :package::rocket: