xjm / drupal_core_release

Set of scripts for preparing a Drupal core release
14 stars 11 forks source link

Version identifier in composer.lock needs to be set correctly when making a release #10

Open hestenet opened 4 years ago

hestenet commented 4 years ago

An unanticipated consequence of the Drupal 8.8.0 release was that the version constraint in composer.lock was not updated (example: https://git.drupalcode.org/project/drupal/blob/8.8.0/composer.lock#L652)

This has a fairly minimal impact: it means that testing won't properly run against a tagged version - however, most often testing is run against the latest -dev. And this can be worked around in DrupalCI. Nevertheless - we should fix.

Suggested fix:

  1. composer require --no-update drupal/core:$VERSION
  2. composer update --lock

This needs testing and validation before being added to this script.

When we know it works - the documentation here should be updated as well: https://www.drupal.org/core/maintainers/create-core-release

Also related - in the long run, it would be great to convert all the steps of the script into a build pipeline that can be triggered on Drupal.org jenkins (or similar) and maintained and edited in a place where any release manager can use it. An issue for that is here: https://www.drupal.org/project/infrastructure/issues/3099100

xjm commented 4 years ago

This will need to be fixed in both tag.sh and sec.sh, once the handbook for the official release process is updated. https://www.drupal.org/core/maintainers/create-core-release The script only automates the official process. Edit: Obviously, once confirming that it actually works as above. :)

Once the handbook is updated, can someone create a PR?

xjm commented 4 years ago

Every time we've set COMPOSER_ROOT_VERSION it's broken the release process. It's been added twice to the script and then removed twice because it prevented both 8.8.0-beta1 and 8.8.0-rc1 from being tagged.

xjm commented 4 years ago

Scenarios that should be manually tested (locally, obviously) if we change the process and scripts:

xjm commented 4 years ago

Also branching 8.1.0x and 9.1.x.

xjm commented 4 years ago

I tried implementing the recommended changes. Resulted in "Your requirements could not be resolved to an installable set of packages."

Full output:

Enter the D8 release number (e.g. 8.0.6 or 8.1.0-beta2):
8.8.1
Enter the previous D8 release (blank for 8.8.0):

Enter the next stable release (blank for 8.8.2):

Composer installing.
Updating metapackage versions to 8.8.1 and tagging.
./composer.json has been updated
Your requirements could not be resolved to an installable set of packages.
[8.8.x c806072495] Drupal 8.8.1
 2 files changed, 2 insertions(+), 2 deletions(-)
Restoring metapackage versions back to -dev
./composer.json has been updated
Your requirements could not be resolved to an installable set of packages.
[8.8.x 9401c295cb] Back to dev.
 2 files changed, 2 insertions(+), 2 deletions(-)
Directory /Users/xjm/.drush/cache/default exists, but is not         [error]
writable. Please check directory permissions.
xjm commented 4 years ago

Sorry, meant to supply the diff also:

index 7c3de99..677ca6d 100755
--- a/tag.sh
+++ b/tag.sh
@@ -55,6 +55,7 @@ echo "Updating metapackage versions to ${v} and tagging."

 # COMPOSER_ROOT_VERSION causes an error when building metapackages for an RC.
 # COMPOSER_ROOT_VERSION="${b}-dev" composer update --lock --no-progress --no-suggest -n -q
+composer require --no-update drupal/core:"$v"
 composer update --lock --no-progress --no-suggest -n -q
 git commit -am "Drupal $v"
 git tag -a "$v" -m "Drupal $v"
@@ -64,6 +65,7 @@ echo "Restoring metapackage versions back to ${b}-dev"

 # COMPOSER_ROOT_VERSION causes an error when building metapackages for an RC.
 # COMPOSER_ROOT_VERSION="${b}-dev" composer update --lock --no-progress --no-suggest -n -q
+composer require --no-update drupal/core:"${b}-dev"
 composer update --lock --no-progress --no-suggest -n -q
 git commit -am "Back to dev."

Edit: fixed diff.

ryanaslett commented 4 years ago

This doesnt need to be a blocker for 8.8.1, as one of the sole remaining places it is being used is within drupalci, and we have a workaround in place for that. I'll see if I can work out what it is that's not working, but COMPOSER_ROOT_VERSION did work for me for every variant I tested (dev, beta, and full release) when I was modifying them earlier.

xjm commented 4 years ago

11 resolved this for the normal release tagging script. We still need to amend the security release script, verify whether the branching script needs any changes, and then update all three handbook pages based on that.

xjm commented 4 years ago

I tried a similar strategy to #11 for sec.sh, but there's a couple reasons that won't actually give the correct result. Since we're merging changes from a different branch, the old composer.lock might be missing real changes -- e.g. an actual dependency update, or even something like adding a new module that should show up in the metapackages. (The latter is unlikely to happen in a sec release, but the former is a staple.)

xjm commented 4 years ago
index bac852d..a448bcc 100755
--- a/sec.sh
+++ b/sec.sh
@@ -267,8 +267,8 @@ for i in "${!versions[@]}"; do
     git add CHANGELOG.txt
   # D8 and higher need to have the lock file updated prior to tagging.
   else
-    echo -e "\nUpdating metapackage versions to ${version}...\n"
-    COMPOSER_ROOT_VERSION="${branch}-dev" composer update --lock --no-progress --no-suggest -n -q
+    echo -e "\nUpdating metapackage versions and lock file to ${version}...\n"
+    COMPOSER_ROOT_VERSION="$v" composer update drupal/core*
   fi

   echo -e "\nTagging ${version}...\n"
@@ -286,6 +286,7 @@ for i in "${!versions[@]}"; do

   # Fix it by checking out the HEAD version and updating that.
   git checkout HEAD -- "$includes_file"
+
   # For D7 only, merge the changelog entry into HEAD manually.
   if [[ "${major[$i]}" = 7 ]] ; then
     git checkout HEAD -- CHANGELOG.txt
@@ -293,8 +294,11 @@ for i in "${!versions[@]}"; do
     git add CHANGELOG.txt
   # For D8 and higher, fix up the lock file again.
   else
-    echo -e "\nRe-updating metapackage versions for the dev branch...\n"
-    COMPOSER_ROOT_VERSION="${branch}-dev" composer update --lock --no-progress --no-suggest -n -q
+    echo -e "\nRe-updating metapackage versions and lock file for the dev branch...\n"
+    # THIS IS BAD AND WRONG. It will revert things like dependency updates or
+    # added modules from the merge.
+    git checkout HEAD -- composer
+    git checkout HEAD -- composer.lock
   fi

   git commit -am "Merged $version." --no-verify

So the last part there needs to be handled differently, probably with using the N-security branch version instead of HEAD, and then doing composer-y stuff to it to reset the version in the lock file and metapackages to dev things.