-
Notifications
You must be signed in to change notification settings - Fork 29
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
Replace composer.canasta.json with YAML additions #463
base: master
Are you sure you want to change the base?
Replace composer.canasta.json with YAML additions #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation is inefficient and needs to be rethought.
exec($gitApplyCmd); | ||
if ($additionalSteps !== null) { | ||
foreach ($steps as $additionalSteps) { | ||
if ($steps === "composer update"){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really a good idea to run composer update once per extension? Shouldn’t these changes be applied all at once and then composer update
be run once at the very end? I can’t support an O(n) implementation for what could easily be an O(1) operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to run "composer update" once per extension/skin, yes - it's less hacky, and allows for more flexibility. (And technically, I think it's O(n) either way, though I wouldn't be surprised if doing it all via a single Composer JSON file, i.e. the current way, is indeed faster.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is, there’s a lot of one-time setup and breakdown when you call composer update. So it takes longer to call composer update 10 times, one update per call, than to call it once with 10 updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can’t in good conscience approve this current implementation.
@yaronkoren: This is an implementation issue with this PR, not a general problem with the proposal, so I don’t see the point of doing that. |
Not really... I think the whole issue of whether to keep the current "composer.canasta.json" setup is part of the design aspect. But if you really want to, we can talk about it here. |
That’s the thing - I don’t have a problem with doing away with “composer.canasta.json”, but I do have a problem with the way this current implementation addresses it. In essence, I am just asking for this current attempt to be refactored so |
I think the only way that can be done is by generating composer.local.json on the fly, and then calling "composer update" on it. Which is certainly doable, and I thought about going with that approach. I still think this approach is better, though, because it just feels like less of a hack - there's one less "moving part", and we're calling Composer the way it's meant to be called. (It's notable that the Composer merge plugin, as useful as it is, is a Wikimedia invention.) If there's a problem with one of the Composer JSON files, it would presumably be easier to detect if they're being called one at a time. And I don't think performance is a real issue - assuming this approach is a few seconds slower, it will have no real impact on the user experience. |
I'm getting very nervous about these small changes being made here and there while this discussion is ongoing. This indicates when the PR was initially released, the code was not complete. By definition, the PR should only be created when the code is completely ready to be merged. Could we have complete code coverage for all of these changes please? I expect to see comprehensive test cases with PHPUnit before I give my approval. |
Sorry, I don't understand this. Changes shouldn't be made to a PR? You've requested changes to this PR yourself. And why does this PR require a test script, while no previous one did? Not that I'm against test scripts - I agree it would be great to have them. |
That's different. Making changes in response to comments is very different from requesting a review and then continuing to make nontrivial changes to achieve code completion.
This is a logical fallacy. Just because we didn't follow good practice in the past doesn't give us an excuse to keep this practice going. Why am I now requesting they be included? Because the code here clearly wasn't code complete when the PR was published, and I've lost my trust in the quality of the code that's being delivered. If the PR wasn't ready for review, then it should've been kept in draft mode and nobody should have been requested to review it. This was not an issue in the past, but it has gotten to the point where it's become an issue I can't overlook anymore. In summary, you can consider this the equivalent of the FAA no longer giving Boeing blanket approvals for 737 MAX certifications. |
Okay, thanks for the explanations. I'm pretty sure what I wrote wasn't a logical fallacy, but that's kind of a minor point. |
@naresh-kumar-babu - since performance is still a concern: could you run this code again, and time how long it takes for all the extensions and skins to be installed? (Or, if it's easier, how long the entire process takes?) |
Time taken by existing approach: 258s |
@naresh-kumar-babu - that's very unexpected! I certainly would never have guessed that calling "composer update" multiple times reduces the execution time. Do you have any idea why this is the case? Could it be that the "merge" plugin runs slowly, due to the complexity of handling everything at once? Or maybe this is just a fluke outcome, because the execution time varies each time? |
Actually, another question: is this just the time taken to install extensions and skins, or the time for the overall setup? |
Time taken to install extensions and skins. |
I'm not sure. I can run a few more times and verify the timing. |
Yes - if you could run each of them at least one more time, that would be great. |
1st Attempt: 2nd Attempt: |
That's awesome! I'm glad you ran these tests - if we make this change, now we'll never have to wonder whether "the tradeoff was worth it", because it turns out that there is no tradeoff - maybe the "merge" plugin is poorly designed or something, but in any case, this new approach is both easier to understand and faster. This also seems like a good test of the overall code - you have managed to run it consistently without problems. @jeffw16 - what do you think? |
Can Naresh please clarify what he means by “existing approach” versus the “proposed approach”? Does the existing approach refer to what is in the main branch or the coded approach here in the PR? |
Existing approach is what is in the main branch. |
Okay, thanks. In that case, it looks like what I had proposed was not yet tested. As this represents an incremental improvement, I am okay with this, but can you add the following comment at the for loop for future reference so we can speed things up more in the future?
|
Wait - why do you think that approach would be faster? My guess was that the removal of the Composer "merge" was what led to the speedup. |
I think the removal of the Composer Merge plugin plus refactoring it to call I’m happy with the current speed improvements - well done - but we should strive for better in the future. Please add the TODO and I will approve this PR. |
But the alternate approach, of creating a composer.local.json file on the fly, would require using the merge plugin again, no? I would think it would take roughly the same amount of time to run as the current (i.e., with composer.canasta.json) approach. |
No, just get rid of the composer.local.json file and edit the composer.json file. |
Oh, I get it. But that just adds more complexity, no? That is, more things could go wrong. What's the benefit? |
Calling Please, can we stop this meaningless discussion and just add this TODO so we can get this merged in? It is not difficult to add this one comment to the code, and we’re not bound to implementing it either. We can discuss later whenever someone gets the chance to work on this improvement. |
Well, I didn't think calling "composer update" 15+ times would be the most efficient solution either, but apparently there's a chance that it is... I just don't want a "TODO" for something that I don't think makes sense to add. Of course, I could be wrong, but I don't want to put in something that indicates some kind of current consensus. If you think this change should be made, the best approach is to create an issue for it. (Or PR, for that matter.) |
Well, "consider change" is certainly better than "do change", but I still don't see the need for this TODO... Modifying composer.json means modifying the core MediaWiki code, which Canasta tries its best not to do. (I can't remember if Canasta modifies anything, but if so it's minimal.) I don't see how even, say, 60-second speedup of this process would be worth the added complexity and potential confusion - and there's no way that a change to the Composer handling would reduce the time by 60 seconds. (The majority of the extension and skin install time is presumably just fetching all the code via Git.) |
Fixes #460 .