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

composer.json - Generalize version requirements #2

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@totten
Copy link
Member

totten commented May 1, 2018

One of the issues complicating distribution via composer is that the
composer.json for the Civi-D8 module requires a specific version number.
This patch makes the composer.json more general -- for example, v5.1.1 of
civicrm-drupal-8 would require v5.1.1 of civirm-core. This
reduces the amount of administration required for each release.

It's hard to properly test composer metadata changes ahead of time, but I did successfully
initialize an example root project (below) that relied on this dependency:

## Make a basic D8 site.
civibuild create drupal8-empty
cd drupal8-empty

## Track changes to our local `composer.json` so that we can do diffs and rollbacks.
git init .
cp example.gitignore .gitignore
git add .
git commit -m 'Import D8 skeleton'

## For testing purposes, manually add my local dev copy of `civicrm-drupal-8.git`
## (which includes this particular patch).
composer config repositories.civicrm-drupal-8 vcs file:///home/myuser/bknix/build/dmaster/sites/all/modules/civicrm/d8foo

## Manually add the only forked dependency
composer config repositories.zetacomponents-mail vcs https://github.com/civicrm/zetacomponents-mail.git

## Install `civicrm-drupal-8` and the corresponding `civicrm-core` and any other dependencies
composer require civicrm/civicrm-drupal-8 dev-master

## Spot-check some files that should have been downloaded automatically
ls -l vendor/civicrm/
# total 0
# drwxr-xr-x 46 totten staff 1564 May  1 02:02 civicrm-core
# drwxr-xr-x 12 totten staff  408 May  1 01:56 civicrm-cxn-rpc
# drwxr-xr-x 11 totten staff  374 May  1 01:56 civicrm-setup

cat vendor/civicrm/civicrm-core/xml/version.xml
# <?xml version="1.0" encoding="iso-8859-1" ?>
# <version>
#   <version_no>5.2.alpha1</version_no>
# </version>

The key thing that's relevant to this change is that it downloaded the corresponding version of civicrm-core.

composer.json - Generalize version requirements
One of the issues complicating distribution via `composer` is that the
`composer.json` for the Civi-D8 module requires a specific version number.
This patch makes the `composer.json` more general -- for example, v5.1.1 of
`civicrm-drupal-8` would require v5.1.1 of `civirm-core`. This
reduces the amount of administration required for each release.
@borisson

This comment has been minimized.

Copy link
Contributor

borisson commented Aug 22, 2018

This makes a ton of sense IMHO. This does make the maintenance easier!

@monishdeb

This comment has been minimized.

Copy link
Member

monishdeb commented Aug 23, 2018

@totten I have tested this patch in my local and I encountered an issue. Here's what I did:
1. composer config repositories.civicrm-drupal-8 vcs file:///home/monish/www/d8-demo/sites/all/modules/civicrm/civi-d8testmodule
2. composer require civicrm/civicrm-drupal-8 5.5.x-dev
3. Applied this patch to civicrm-drupal-8 the composer.json
3. On composer install

Monishs-MacBook-Pro:civicrm-drupal-8 monish$ composer install
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for civicrm/civicrm-core 5.5.x-dev -> satisfiable by civicrm/civicrm-core[5.5.x-dev].
    - civicrm/civicrm-core 5.5.x-dev requires pear/validate_finance_creditcard dev-master -> satisfiable by pear/validate_finance_creditcard[dev-master] but these conflict with your requirements or minimum-stability.

I think for pear/validate_finance_creditcard need to have self.version against it in civicrm-core's composer.json?

@borisson

This comment has been minimized.

Copy link
Contributor

borisson commented Aug 29, 2018

The more I think about this, to less I feel that doing this is the best path forward. I think adding ~5.4 is the solution we should go for. This will allow everything up to, but not including the next significant release (6.x).

We are already using 5.4.x specific code, so allowing something lower doesn't seem like a great idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment