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

Download all "real" extensions and skins via Git and not Composer #280

Merged
merged 44 commits into from
Aug 25, 2023

Conversation

yaronkoren
Copy link
Member

@yaronkoren yaronkoren commented Aug 18, 2023

The only extensions that are still downloaded via Composer are "helper"/library extensions like DataValues, ParserHooks and Bootstrap (which is to some extent a helper extension).

This PR adds patches for four SMW-based extensions: two (SBL and SRF) get their composer.json requirement for Semantic MediaWiki removed, so that calling "composer install/update" on them does not lead to unnecessary downloads; and the other two (SCQ and SS) get "autoload" added to their extension.json, taking the place of autoload that was called via Composer.

This change should happen concurrently with CanastaWiki/Canasta-DockerCompose#48, which removes all extensions and skins from the file composer.local.json.

The only extensions that are still downloaded via Composer are "helper"/library extensions like DataValues, ParserHooks and Bootstrap (which is to some extent a helper extension).

This change requires a corresponding change to the Canasta-DockerCompose repository to remove all extensions and skins from the file composer.local.json.
Dockerfile Show resolved Hide resolved
@vedmaka
Copy link
Collaborator

vedmaka commented Aug 24, 2023

@cicalese and @vedmaka - is that description correct?

yep (+taking into account @cicalese comment)

And if so, any preferences on what to do? My own inclination is to go with Cindy's recommendation to patch all five extensions

That's right, in this situation, we either patch the extensions extension.json or the composer.json

the less we have to deal with Composer, the better, I think.

I am unsure if dealing less with Composer is the right thing to do in the long term. The discussion about using Composer as a main extensions management tool [1] is still in progress here. It could be that managing all the extensions via composer (even non-Composer ones) is a good idea, especially considering the update problem is somewhat solved here wikimedia/composer-merge-plugin@1b1afcb and here wikimedia/composer-merge-plugin#257

In theory, this provides a good number of benefits, like having a parsable composer file on the image instead of tons of git clone .. commands and a free version and extensions dependencies checks on extensions being installed

@yaronkoren
Copy link
Member Author

Thanks to both of you for your comments. @vedmaka - you may be right that, in the future, a pure Composer-based approach will be the way to go. For now, though, this kind of hybrid approach seems to be causing unnecessary complexity and difficulty, both for admins and for us developers.

I have been making changes to the code and to this PR, based on the recommendations. I think the big thing left to do now is to create patches for SCQ and SS. I'm not sure what to do add to extension.json for either extension - can anyone offer a hint?

@cicalese
Copy link
Contributor

I have been making changes to the code and to this PR, based on the recommendations. I think the big thing left to do now is to create patches for SCQ and SS. I'm not sure what to do add to extension.json for either extension - can anyone offer a hint?

I was going to work on patches for those two, but I'm currently having trouble running with this patch. I see that there is a merge conflict in the composer.canasta.json file that needs to be resolved. Could you please fix that? The error I'm getting is that it is missing Vector, though, so I'm not sure that's related. I'll let you know if I determine there's another issue.

@yaronkoren
Copy link
Member Author

Oh yeah, I should have noticed that merge conflict. I just fixed it, I think.

@cicalese
Copy link
Contributor

cicalese commented Aug 24, 2023

I think the big thing left to do now is to create patches for SCQ and SS. I'm not sure what to do add to extension.json for either extension - can anyone offer a hint?

For SemanticCompoundQueries/extension.json:

  • replace:
    "load_composer_autoloader":true,
  • with:
	"AutoloadNamespaces": {
		"SCQ\\": "src/"
	},
	"AutoloadClasses": {
		"SemanticCompoundQueries": "SemanticCompoundQueries.php"
	},

For SemanticScribunto/extension.json:

  • replace:
    "load_composer_autoloader":true,
  • with:
	"AutoloadNamespaces": {
		"SMW\\Scribunto\\": "src/"
	},
	"AutoloadClasses": {
		"SemanticScribunto": "SemanticScribunto.php"
	},

I successfully tested these modifications on my local instance.

@yaronkoren
Copy link
Member Author

Okay, thanks, that's great! Are you sure that the "load_composer_autoloader" lines to be removed, though? They're there for some of the other extensions:

https://github.com/SemanticMediaWiki/SemanticTasks/blob/master/extension.json#L35
https://github.com/SemanticMediaWiki/SemanticFormsSelect/blob/master/extension.json#L66

@cicalese
Copy link
Contributor

Okay, thanks, that's great! Are you sure that the "load_composer_autoloader" lines to be removed, though? They're there for some of the other extensions:

https://github.com/SemanticMediaWiki/SemanticTasks/blob/master/extension.json#L35 https://github.com/SemanticMediaWiki/SemanticFormsSelect/blob/master/extension.json#L66

Yes. See https://www.mediawiki.org/wiki/Manual:Extension.json/Schema#load_composer_autoloader. Since those extensions do not have any dependencies on libraries that are specified in composer.json, that line can/should be removed. It should actually be removed from the other extensions you referenced as well, but that's a problem for another day.

@vedmaka
Copy link
Collaborator

vedmaka commented Aug 24, 2023

By the way, I believe the load_composer_autoloader documentation is not well worded. This flag forces local ./vendor/autoload.php from the extension directory to be loaded. This has no use when the merge-plugin is in place because when the extension Composer file is loaded through the merge-plugin all the dependencies are added to the main autoload file and no vendor is present at the extension directory

@yaronkoren
Copy link
Member Author

I just added the autoload patches for SCQ and SS to this PR - including the "load_composer_autoloader" removal. If it turns out that this line is harmless, then it will be easy to, er, remove that removal. In any case, assuming this all works, it will be good to at least add those new autoload lines "upstream" to the SCQ and SS extensions.

@github-actions
Copy link

🐳 The image based on d73675ae commit has been built with 1.39.1-20230824-280 tag as ghcr.io/canastawiki/canasta:1.39.1-20230824-280

@cicalese cicalese self-requested a review August 24, 2023 23:20
@cicalese
Copy link
Contributor

Tested. LGTM.

@cicalese cicalese requested a review from vedmaka August 24, 2023 23:22
@yaronkoren
Copy link
Member Author

@vedmaka - it looks like this PR is ready to be merged in, but I can't merge it in unless you cancel your requested change. Could you do that, assuming you think this current code is fine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants