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

Update branch alias for 1.6.x #1262

Merged
merged 1 commit into from May 9, 2023
Merged

Conversation

gmazzap
Copy link
Contributor

@gmazzap gmazzap commented May 8, 2023

The composer.json's branch alias for 1.5.x points to 1.4.x-dev version.

This is making requirements for the next unreleased 1.5.x impossible, unless explictly requiring the branch.

@ghostwriter ghostwriter added waiting-for-feedback Patch Backwards compatible bug fixes and improvements Chore for non-functional changes or maintenance tasks such as code cleanup, formatting, or dependency upda labels May 8, 2023
Copy link
Member

@ghostwriter ghostwriter left a comment

Choose a reason for hiding this comment

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

Hey @gmazzap,

Thanks for bringing this to our attention.

  1. The next unreleased branch is 1.6.x.
  2. I recommend using the approach you mentioned, explicitly requiring the branch name.
  3. I think we should remove the branch alias from the composer configuration
    • remove maintenance burden
    • redundant functionality as explicitly requiring the branch name
    • document this somewhere

The branching model that we are currently using will maintain a SemVer-like [0-9]+.[0-9]+.x branch.

Next Minor

{
    "require-dev": {
        "mockery/mockery": "1.6.x-dev"
    },
}

Next Major

{
    "require-dev": {
        "mockery/mockery": "2.0.x-dev"
    },
}

You can also install and test that branch by using this composer command.

composer require --dev mockery/mockery:1.6.x-dev and composer require --dev mockery/mockery:2.0.x-dev

What do you think?

@gmazzap
Copy link
Contributor Author

gmazzap commented May 8, 2023

Hi @ghostwriter

I'm sorry, but I don't understand.

You have specific branches for specific minor releases.

However, you're telling Composer, via branch alias, that both your 1.5.x and 1.6.x branches contain code comparable with 1.4.x versions. That's just not true and will leads to issues.

If there're two packages in the same installation, one requiring 1.4.* and another requiring 1.6.x-dev , Composer will think it can pull the 1.6.x branch and satisfy both, when it is clear that the package requiring 1.4.* does not want 1.6.x code.

The correct branch alias should be:

"branch-alias": {
     "dev-master": "1.4.x-dev",
     "1.5.x": "1.5.x-dev",
     "1.6.x": "1.6.x-dev",
}

That way, assuming minimum stability "dev", if a package requires 1.4.* and another package requires dev-master, Composer would pull the master branch satisfying both.
If a package requires, for example, ^1.5 and another requires dev-master Composer will correctly refuse to install, because the master branch does not have code comparable with v1.5. But if the two requirements to satisfy would be dev-1.5.x and ^1.5 the installation would correctly success.


I'm not going to require branch explictly. If I require 1.5.x-dev, as soon as you'll release 1.6, I'm not gonna get it. And if I require 1.6.x-dev, as soon as you'll release 1.7, I'm not going to get that.
If you setup your branch alias to reflect the reality (as I proposed above), I can now require ^1.5.1 and I'll get the code of 1.5.1 tag if my minimum stability is "stable", and code from 1.5.x branch if my minimum stability is "dev".
That's the whole point of branch alias.
But as soon as you'll release 1.6.0 or 1.7.0 my ^1.5.1 requirement will get the new version without me touching the requirement. Requiring the branch explictly means I have to continously update the requirement, and that's non-starter for me.

With the current situation, I'll keep requiring ^1.5 as I am now, but that means I need to wait the release of next tag to get code that is already in the 1.5.x and makes test compatible with PHP 8.2. It's a bit annoying but I guess I can wait.

@ghostwriter
Copy link
Member

ghostwriter commented May 9, 2023

Hi @ghostwriter

I'm sorry, but I don't understand.

No worries, I'll try to explain.


You have specific branches for specific minor releases.

Correct. ✅


However, you're telling Composer, via branch alias, that both your 1.5.x and 1.6.x branches contain code comparable with 1.4.x versions. That's just not true and will leads to issues.

If there're two packages in the same installation, one requiring 1.4.* and another requiring 1.6.x-dev , Composer will think it can pull the 1.6.x branch and satisfy both, when it is clear that the package requiring 1.4.* does not want 1.6.x code.

Correct! ✅ That's the bug.


The correct branch alias should be:

"branch-alias": {
     "dev-master": "1.4.x-dev",
     "1.5.x": "1.5.x-dev",
     "1.6.x": "1.6.x-dev",
}

That way, assuming minimum stability "dev", if a package requires 1.4.* and another package requires dev-master, Composer would pull the master branch satisfying both. If a package requires, for example, ^1.5 and another requires dev-master Composer will correctly refuse to install, because the master branch does not have code comparable with v1.5. But if the two requirements to satisfy would be dev-1.5.x and ^1.5 the installation would correctly success.

Correct! ✅ (however, I think we should instead remove the branch-alias configuration from the composer.json file, which would solve the bug we mentioned above.)


I'm not going to require branch explictly. If I require 1.5.x-dev, as soon as you'll release 1.6, I'm not gonna get it.

Correct! ✅ The 1.5.x branch will only contain tags between 1.5.0-alpha and 1.5.[0-9]+.

If you want the changes tagged in the 1.6.x branch you have to explicitly use 1.6.x-dev.


And if I require 1.6.x-dev, as soon as you'll release 1.7, I'm not going to get that.

Correct! ✅ That's the intended behavior. All development and tags are created in their respective major/minor versioned branches.

If you setup your branch alias to reflect the reality (as I proposed above), I can now require ^1.5.1 and I'll get the code of 1.5.1 tag if my minimum stability is "stable", and code from 1.5.x branch if my minimum stability is "dev".
That's the whole point of branch alias. But as soon as you'll release 1.6.0 or 1.7.0 my ^1.5.1 requirement will get the new version without me touching the requirement.

Correct! ✅ but unfortunately ⛔️ Bad practice, you should never use non-existing tags.

The reason there is a separate branch for 1.5.x-dev and 1.6.x-dev is that the 1.6.x-dev branch adds a new feature in a backward-compatible manner (meaning the new feature does not change any of the old behavior).

  • ⛔️ Based on your statement, if you require ^1.5.1 now and 1.5.1 is not released and instead a new minor ^1.6.0you will have to manually update the version constraints
  • ⛔️ if the branch-alias is updated to target another branch while you're still requiring a non-existing tag.

Requiring the branch explictly means I have to continously update the requirement, and that's non-starter for me.

Incorrect ❌ Explicitly requiring the 1.6.x-dev branch would allow for the use of the initial minor version and patches (1.6.0, 1.6.1, 1.6.999, etc...) before they are released.

So before/when/if we tag and release 1.7.0, it's maintained on the 1.7.x-dev branch.


With the current situation, I'll keep requiring ^1.5 as I am now, but that means I need to wait the release of next tag to get code that is already in the 1.5.x and makes test compatible with PHP 8.2. It's a bit annoying but I guess I can wait.

Incorrect ❌

You can test compatibility with PHP 8.2 using:

{
    "require-dev": {
        "mockery/mockery": "^1.5 || 1.6.x-dev"
    },
}

or

{
    "require-dev": {
        "mockery/mockery": "1.6.x-dev"
    },
}

or

{
    "require-dev": {
        "mockery/mockery": "1.6.x-dev as 1.6.0"
    },
}

And once 1.6.0 is released use:

{
    "require-dev": {
        "mockery/mockery": "^1.6"
    },
}

Let me know if I misunderstood anything or if this clears up some things or not.

@ghostwriter ghostwriter self-assigned this May 9, 2023
@ghostwriter ghostwriter changed the title Update branch alias for 1.5.x Update branch alias for 1.6.x May 9, 2023
@ghostwriter ghostwriter changed the base branch from 1.5.x to master May 9, 2023 12:59
@ghostwriter ghostwriter changed the base branch from master to 1.5.x May 9, 2023 14:40
@ghostwriter ghostwriter changed the base branch from 1.5.x to master May 9, 2023 14:47
`composer.json`'s branch alias for 1.6.x points to 1.4.x-dev version.

Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Copy link
Member

@ghostwriter ghostwriter left a comment

Choose a reason for hiding this comment

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

My previous comment is still valid, however, I have decided to proceed to merge this pull request.

Upon further reflection, I believe it would be more appropriate to remove the branch-alias in the 2.0.x version, considering the presence of other breaking changes.

  • Increased maintenance overhead:
    • Branch aliases require continuous monitoring and updating to ensure that the aliases point to the correct branches and are compatible with other dependencies.
    • Adds an additional layer of complexity to dependency management.
    • Requires proper documentation and communication to ensure developers understand the branch aliases used.
"require": {
    "vendor/package": "dev-main#<commit-hash>"
},
"extra": {
    "branch-alias": {
        "dev-main": "1.0.x-dev"
    }
}

This maintenance overhead can be time-consuming, especially when dealing with multiple projects and their respective branch aliases.


In our branching model, we now have a dedicated branch for each major and minor version of Mockery maintained (SemVer major.minor.x)

This will allow developers to directly specify the branch name as a dependency in the composer.json file.

"require": {
    "vendor/package": "dev-branch-name"
}

No need for extra configuration or understanding of branch-alias.


This PR will target the master branch so that developers with similar expectations will be satisfied.

Thank you, for your contribution.

P.S. I'm happy to continue the conversation going to see what others have to say on this subject.

@ghostwriter ghostwriter merged commit 612a8f9 into mockery:master May 9, 2023
4 of 5 checks passed
@ghostwriter
Copy link
Member

PHP 8.2 CI check is expected to fail, unrelated to this PR.

@ghostwriter ghostwriter added this to the 1.6.0 milestone Jun 7, 2023
@ghostwriter ghostwriter mentioned this pull request Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chore for non-functional changes or maintenance tasks such as code cleanup, formatting, or dependency upda Patch Backwards compatible bug fixes and improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants