Don't include composer files already required by root package #251

wgevaert · 2022-10-31T08:48:39Z

This solves several issues, including but not limited to the following:
Make a composer.json similar to this:

{
    "require": {
        "wikimedia/composer-merge-plugin": "dev-master"
    },
    "extra": {
        "merge-plugin": {
            "include": [
                "composer.local.json",
                "extensions/*/composer.json"
            ],
            "recurse": true
        }
    }
}

With this set up, the following composer.local.jsons give problems:

{
  "require": {
    "mediawiki/user-functions": "dev-REL1_35"
  },
  "extra": {
    "merge-plugin": {
      "include": [
        "extensions/*/composer.json"
      ]
    }
  }
}

Gives the error Fatal error: Cannot redeclare <some function/class> (previously declared in <directory>/extensions/<name>/<file>:<line>) in <directory>/extensions/<name>/<file> on line <line>, as described in this issue on this repo and this issue in the composer repo

{
    "require": {
        "mediawiki/semantic-media-wiki": "3.2.3"
    },
    "extra": {
        "merge-plugin": {
            "include": [
                "extensions/*/composer.json"
            ]
        }
    }
}

Gives the error

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - my/root-package is present at version 1.0.0 and cannot be modified by Composer
    - mediawiki/semantic-media-wiki[3.2.3] cannot be installed as that would require removing my/root-package[1.0.0]. They both replace mediawiki/semantic-mediawiki and thus cannot coexist.
    - Root composer.json requires mediawiki/semantic-media-wiki 3.2.3 -> satisfiable by mediawiki/semantic-media-wiki[3.2.3].

which is also discussed in this issue on this repo (Which also says "[..] it would be good if there was some way to indicate not to merge the configs of any packages that are already included by the main composer.json file since their configs will already be evaluated by Composer when they're required in the main composer.json file.", which is what this pull request does). Solving the issue by using the merge-replace: false option gives the same Fatal error: Cannot redeclare problem described above.

This pull request solves all these issues.

this solves several issues

apoca · 2023-01-06T16:51:20Z

I have the same issue here. When do we have news about this issue!?

apoca · 2023-01-10T21:35:00Z

Can someone look into this?

apoca · 2023-01-12T23:49:08Z

Hi, @wgevaert I've tested your code locally and it doesn't work I have the same error here and more...

physikerwelt · 2023-07-24T15:23:44Z

@reedy This solves my problem with duplicate includes.

see above

Fatal error: Cannot redeclare <some function/class> (previously declared in <directory>/extensions/<name>/<file>:<line>) in <directory>/extensions/<name>/<file> on line <line>

tested by modifying my composer.json file that way:

diff --git a/composer.json b/composer.json
index 9cd6b1ed536..27d9750e402 100644
--- a/composer.json
+++ b/composer.json
@@ -20,6 +20,11 @@
                "wiki": "https://www.mediawiki.org/"
        },
        "prefer-stable": true,
+       "repositories": [
+               {
+                       "url": "https://github.com/wgevaert/composer-merge-plugin.git",
+                       "type": "git"
+               }],
        "require": {
                "composer/semver": "3.3.2",
                "cssjanus/cssjanus": "2.1.1",
@@ -57,7 +62,7 @@
                "wikimedia/cdb": "2.0.0",
                "wikimedia/cldr-plural-rule-parser": "2.0.0",
                "wikimedia/common-passwords": "0.5.0",
-               "wikimedia/composer-merge-plugin": "2.1.0",
+               "wikimedia/composer-merge-plugin": "dev-bugfix-don-t-include-required",
                "wikimedia/html-formatter": "3.0.1",
                "wikimedia/ip-set": "3.1.0",
                "wikimedia/ip-utils": "4.0.0",

wgevaert · 2023-07-26T07:55:11Z

phiskerwelt wrote:

The only issue I see with this patch is that it silently fixes problems occured somewhere else. Maybe adding a warning on duplicate values would resolve the concerns by the package maintainers. I hope after summer break we will get some feedback

I do give a "warning" here, but currently it is "info" (only seen when running composer in verbose mode). Should I change to "warning"? It might be spammy.

physikerwelt · 2023-07-26T08:56:42Z

Sorry, I was not clear. With this, I was referring to the change to composer.

https://github.com/composer/composer/pull/11109/files#diff-67d1dfefa9c7b1c7e0b04b07274628d812f82cd82fae635c0aeba643c02e8cd8R668

Change #251 should be merged as it is, I think.

physikerwelt · 2023-07-26T09:13:16Z

After thinking about it for a bit, I asked myself if there are other extensions (not SMW) that cause this problem. Otherwise, one could discuss the way how SMW is installed. Why is it different from other extensions, that you just download and where you install the dependencies via composer? Or if it insists on being installed via composer, why can it not be stored in the vendor folder?

wgevaert · 2023-07-26T09:25:17Z

After thinking about it for a bit, I asked myself if there are other extensions (not SMW) that cause this problem. Otherwise, one could discuss the way how SMW is installed. Why is it different from other extensions, that you just download and where you install the dependencies via composer? Or if it insists on being installed via composer, why can it not be stored in the vendor folder?

SMW is not the only extension that can be installed using composer, see this page

To name a few.

physikerwelt · 2023-07-26T09:29:53Z

Thank you. This means that there is a critical mass to care about. However, https://phabricator.wikimedia.org/T250406 is not decided.

Krinkle · 2023-07-26T15:28:13Z

With this set up, the following composer.local.jsons give problems:

{
  "require": {
    "mediawiki/user-functions": "dev-REL1_35"
  },
  "extra": {
    "merge-plugin": {
      "include": [
        "extensions/*/composer.json"
      ]
    }
  }
}

From the perspective of what is officially supported, I believe the "problem" is the inclusion of "mediawiki/user-functions" to fetch extensions. Per RFC T467 this is not officially supported. If you remove that, and make sure to have a Git clone of the extension, then everything should automatically work.

Having said that, there's clearly a specific subset of extensions that have never stopped advertising and internally making use of Composer to manage their own installation. I think for people that prefer that, that can probably unofficially work. But, then you must remove the use of "extensions/*/composer.json" and instead fully embrace the "Composer way", i.e. use "require" to list out each extension you want to install (if it is published there), and then for extensions that aren't published there (i.e. most) you clone them from Git and list them by name under "include" — not with a wildcard.

There is no concept native to Composer of magically by-wildcard depending on packages that happen to exist on disk. If you want to use Composer to install extensions, then I think you have to go all-in on that and list out each extension want to install (via "require" or "include", no wildcard) in your mediawiki/composer.local.json file.

There is one other thing that stands out to me, which I don't fully understand:

Make a composer.json similar to this:
{
            …
                "extensions/*/composer.json"
            …
}
With this set up, the following composer.local.json give problems:
{
            …
        "extensions/*/composer.json"
            …
}

Do I understand correctly that this first composer.json file is the mediawiki root composer.json? I have not seen before that someone would modify this, and certainly not include extensions/*/ there as well. If I understood this correctly, may I ask what the motivation is for adding a wildcard in both files? It believe we usually put the wildcard (only) in composer.local.json.

hexmode · 2023-07-26T19:07:25Z

fwiw, I agree with this from @Krinkle:

you must remove the use of "extensions/*/composer.json" and instead fully embrace the "Composer way", i.e. use "require" to list out each extension you want to install

With this setup run the update and see the duplicate import problem cf. wikimedia/composer-merge-plugin#251

physikerwelt · 2023-07-27T06:02:08Z

@Krinkle, I understand that various problems can be resolved with this PR. I also do not fully understand all the problems described here. However, without any extensions downloaded the following composer.local.json leads to

{
	"require": {
        "mediawiki/semantic-media-wiki": "~4.1"
    },
	"extra": {
		"merge-plugin": {
			"include": [
				"extensions/*/composer.json"
			]
		}
	}
}

Fatal error: Cannot redeclare <some function/class> (previously declared in <directory>/extensions/<name>/<file>:<line>) in <directory>/extensions/<name>/<file> on line <line> when running the updater or visiting the wiki.

Krinkle · 2023-07-27T17:25:26Z

@physikerwelt Yes, my recommendation is to follow either the officially supported way, or to use Composer. Right now it is mixing them, which leads to the error.

The supported way, as documented in composer.local.json-sample, is for the local file to contain only the following:

{
	"extra": {
		"merge-plugin": {
			"include": [
				"extensions/*/composer.json"
			]
		}
	}
}

This means you control what's installed by cloning extensions, and should work for all extensions, including SMW.

The Composer way (which is not officially supported, but should work!), is to specify each extension you want to install separately. This is happens naturally when you use "require" as wildcards are not valid there. For non-composer extensions, you'd list them in the "include" like so:

{
	"require": {
		"mediawiki/semantic-media-wiki": "~4.1"
	},
	"extra": {
		"merge-plugin": {
			"include": [
				"extensions/AbuseFilter/composer.json",
				"extensions/AntiSpood/composer.json"
			]
		}
	}
}

I haven't reviewed the patch in detail, but while I believe it would likely be safe and not break any existing installs, my worry is that removes a useful ability to detect mistakes, unless this is the only possible way to trigger that error? Otherwise, it would silence the one way to detect such an issue.

physikerwelt · 2023-10-14T09:07:07Z

The related change to warn about duplicate files has been merged composer/composer#11109 . However, the problem seems to be still there. When thinking about this again, I am wondering why SMW installs itself to extensions and not to vendor.

wgevaert · 2023-10-14T09:50:09Z

When thinking about this again, I am wondering why SMW installs itself to extensions and not to vendor.

Because it is a mediawiki extension. See the composer-installer plugin: packages with "type": "mediawiki-extension" are installed into the extension folder, and mediawiki-skin to the skin folder.

physikerwelt · 2023-10-24T05:45:20Z

When thinking about this again, I am wondering why SMW installs itself to extensions and not to vendor.

Because it is a mediawiki extension. See the composer-installer plugin: packages with "type": "mediawiki-extension" are installed into the extension folder, and mediawiki-skin to the skin folder.

This is how it is implemented, I was wondering why it was done.

physikerwelt · 2023-11-25T11:47:43Z

While SMW has quite an extensive installation guide, there is no mention of installing it from git. But it works by cloning https://github.com/SemanticMediaWiki/SemanticMediaWiki/. So for me, there is no issue anymore.

Don't include composer files already required by root package

18a80ac

this solves several issues

valerio-bozzolan mentioned this pull request Apr 25, 2023

Double inclusion of certain files which throws errors. #247

Open

wgevaert mentioned this pull request Jul 24, 2023

Add array_unique call to prevent loading the same file multiple times. composer/composer#11109

Merged

physikerwelt approved these changes Jul 24, 2023

View reviewed changes

physikerwelt added a commit to MaRDI4NFDI/docker-wikibase that referenced this pull request Jul 27, 2023

Find minimal example to reproduce composer problem

8330af2

With this setup run the update and see the duplicate import problem cf. wikimedia/composer-merge-plugin#251

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't include composer files already required by root package #251

Don't include composer files already required by root package #251

wgevaert commented Oct 31, 2022 •

edited

Loading

apoca commented Jan 6, 2023 •

edited

Loading

apoca commented Jan 10, 2023

apoca commented Jan 12, 2023

physikerwelt commented Jul 24, 2023

wgevaert commented Jul 26, 2023

physikerwelt commented Jul 26, 2023 •

edited

Loading

physikerwelt commented Jul 26, 2023

wgevaert commented Jul 26, 2023 •

edited

Loading

physikerwelt commented Jul 26, 2023

Krinkle commented Jul 26, 2023 •

edited

Loading

hexmode commented Jul 26, 2023

physikerwelt commented Jul 27, 2023

Krinkle commented Jul 27, 2023

physikerwelt commented Oct 14, 2023

wgevaert commented Oct 14, 2023

physikerwelt commented Oct 24, 2023

physikerwelt commented Nov 25, 2023

Don't include composer files already required by root package #251

Are you sure you want to change the base?

Don't include composer files already required by root package #251

Conversation

wgevaert commented Oct 31, 2022 • edited Loading

apoca commented Jan 6, 2023 • edited Loading

apoca commented Jan 10, 2023

apoca commented Jan 12, 2023

physikerwelt commented Jul 24, 2023

wgevaert commented Jul 26, 2023

physikerwelt commented Jul 26, 2023 • edited Loading

physikerwelt commented Jul 26, 2023

wgevaert commented Jul 26, 2023 • edited Loading

physikerwelt commented Jul 26, 2023

Krinkle commented Jul 26, 2023 • edited Loading

hexmode commented Jul 26, 2023

physikerwelt commented Jul 27, 2023

Krinkle commented Jul 27, 2023

physikerwelt commented Oct 14, 2023

wgevaert commented Oct 14, 2023

physikerwelt commented Oct 24, 2023

physikerwelt commented Nov 25, 2023

wgevaert commented Oct 31, 2022 •

edited

Loading

apoca commented Jan 6, 2023 •

edited

Loading

physikerwelt commented Jul 26, 2023 •

edited

Loading

wgevaert commented Jul 26, 2023 •

edited

Loading

Krinkle commented Jul 26, 2023 •

edited

Loading