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

Use stable branch for --edge option when possible #41568

Merged
merged 1 commit into from Mar 7, 2021

Conversation

jonathanhefner
Copy link
Member

This alleviates the need to update the code when there is a new stable branch (for example, as done in #41454).

@rails-bot rails-bot bot added the railties label Feb 27, 2021
This alleviates the need to update the code when there is a new stable
branch (for example, as done in rails#41454).
@@ -276,8 +276,9 @@ def rails_gemfile_entry
GemfileEntry.path("rails", Rails::Generators::RAILS_DEV_PATH)
]
elsif options.edge?
edge_branch = Rails.gem_version.prerelease? ? "main" : [*Rails.gem_version.segments.first(2), "stable"].join("-")
Copy link
Member

Choose a reason for hiding this comment

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

Big +1 to anything that reduces the set of things we need to change during the branch/release process.

@jonathanhefner I think the condition here might want to be something more like Rails.gem_version < Gem::Version.new(Rails.gem_version.segments.first(2).join(".")) (though hopefully there's a nicer way to spell that?)

.. or if we want to really codify [my recollection of] recent practice, + ".0.rc1"

Copy link
Member Author

Choose a reason for hiding this comment

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

@matthewd I believe that will give the same result:

ns = %w[0 1 2]
versions = ns.product(ns, [*ns, "0.alpha", "0.rc1", "0.rc2", "0.1"]).
  map { |segments| Gem::Version.new(segments.join(".")) }

versions.all? { |v| v.prerelease? == v < Gem::Version.new(v.segments.first(2).join(".")) }
# => true

I had considered checking for rc vs alpha, but I wasn't sure if an x-y-stable branch was guaranteed to exist immediately after an x.y.0.rc1 release. I thought I remembered a lag in the past, though perhaps not. If not, I can change this to:

version = Rails.gem_version
edge_branch = version < Gem::Version.new("#{version.release}.rc1") ? "main" : [*version.segments.first(2), "stable"].join("-")

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that 5.2.3.rc2 (or indeed 5.2.3.beta1) should use 5-2-stable, never main -- despite it being a prerelease of 5.2.3, the 5.2 series is established, and by that point it's highly likely main has diverged in notable ways.

Gem::Version.new("5.2.3.rc2").prerelease? # => true
Gem::Version.new("5.2.3.rc2") < Gem::Version.new("5.2") # => false

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point, though I don't think an x-y-stable branch will exist when an x.y.0.beta1 is released. But perhaps it would be best to just blow up in that case. And likewise for an x.y.0.rc1 release if x-y-stable does not exist. To that end, I've submitted #41837. It changes the condition to target main branch only for alpha versions, and, additionally, to blow up when the targeted stable branch does not exist (rather than continuing on to install webpack).

jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Apr 4, 2021
Follow-up to rails#41568.

If a user runs `rails new --edge` with an `x.y.0.betaN` or `x.y.z.rcN`
version, they most likely want to track the `x-y-stable` branch rather
than `main` branch.  In some cases, the `x-y-stable` branch may not
exist yet.  In those cases, it is better to raise an error when `bundle
install` fails, so that the user knows that the branch is not available.
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