Skip to content

Sort app scripts topologically by its dependencies #30385

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 29, 2021
Merged

Sort app scripts topologically by its dependencies #30385

merged 1 commit into from
Dec 29, 2021

Conversation

@mejo-
Copy link
Member

@mejo- mejo- commented Dec 23, 2021

Implement a proper topological sorting algorithm. Based on the
implementation by https://github.com/marcj/topsort.php

Throws a CircularDependencyException if a circular dependency is
detected.

Fixes: #30278

@mejo- mejo- mentioned this pull request Dec 23, 2021
Closed
@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch 3 times, most recently from 3ddb89f to 9e22367 Compare December 23, 2021 10:01
@mejo-
Copy link
Member Author

mejo- commented Dec 23, 2021

Not sure whether we want the CircularDependencyException to be thrown. We also could skip that if we don't consider circular dependencies a problem (in certain cases they can result in broken dependency chains).

@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch from 9e22367 to ed247d9 Compare December 23, 2021 10:15
@mejo-
Copy link
Member Author

mejo- commented Dec 23, 2021

Not sure whether we want the CircularDependencyException to be thrown. We also could skip that if we don't consider circular dependencies a problem (in certain cases they can result in broken dependency chains).

Thinking about it again, it's not a good idea to fail hard here by throwing an exception and we should just log an error instead. Agreed?

@mejo-
Copy link
Member Author

mejo- commented Dec 23, 2021

Thinking about it again, it's not a good idea to fail hard here by throwing an exception and we should just log an error instead. Agreed?

I removed the CircularDependencyException. Instead, the circular dependency is now merely logged as an error if detected.

@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch 2 times, most recently from 571a872 to 0690078 Compare December 23, 2021 15:55
skjnldsv
skjnldsv previously approved these changes Dec 27, 2021
View reviewed changes
ChristophWurst
ChristophWurst requested changes Dec 27, 2021
View reviewed changes
@skjnldsv skjnldsv self-requested a review December 27, 2021 09:57
@skjnldsv skjnldsv dismissed their stale review December 27, 2021 09:57

see Christoph remarks

@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch 2 times, most recently from 3e7f920 to 71f5da2 Compare December 27, 2021 11:26
@mejo- mejo- requested a review from ChristophWurst December 27, 2021 12:18
@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch from 0b5adc8 to 5c2e069 Compare December 27, 2021 12:22
@juliusknorr juliusknorr mentioned this pull request Dec 28, 2021
Merged
@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch 3 times, most recently from 221d607 to bcc4c71 Compare December 28, 2021 18:19
@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch from bcc4c71 to e438f97 Compare December 28, 2021 18:55
skjnldsv
skjnldsv reviewed Dec 29, 2021
View reviewed changes
skjnldsv
skjnldsv approved these changes Dec 29, 2021
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This took a crazy turn! Code looks good, tests too! 🚀
Awesome work!

@juliusknorr
Copy link
Member

Psalm failure unrelated

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Dec 29, 2021
@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch 2 times, most recently from 4a94d01 to 29df162 Compare December 29, 2021 13:49
ChristophWurst
ChristophWurst approved these changes Dec 29, 2021
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 :shipit:

@mejo-
Copy link
Member Author

mejo- commented Dec 29, 2021

This took a crazy turn!

Soooo true 🙈 🤪

I wouldn't have expected this to become such a ride. Thanks a lot to @ChristophWurst and @skjnldsv for guiding me!

@mejo-
Sort app scripts topologically by its dependencies
491bd62 
Implement a proper topological sorting algorithm. Based on the
implementation by https://github.com/marcj/topsort.php

Logs an error in case a circular dependency is detected.

Fixes: #30278

Signed-off-by: Jonas Meurer <jonas@freesources.org>
@mejo- mejo- force-pushed the fix/30278/sort_algorithm branch from 29df162 to 491bd62 Compare December 29, 2021 15:41
@mejo-
Copy link
Member Author

mejo- commented Dec 29, 2021

Seems like I'm not allowed to merge - maybe due to the (unrelated) psalm test failures? 🤔 Could someone with super powers merge? 😬

@ChristophWurst ChristophWurst merged commit 8eee11e into master Dec 29, 2021
@ChristophWurst ChristophWurst deleted the fix/30278/sort_algorithm branch December 29, 2021 16:23
@artonge artonge mentioned this pull request Jan 5, 2022
Merged
CarlSchwan
CarlSchwan reviewed Jan 6, 2022
View reviewed changes
lib/public/Util.php

namespace OCP;

use OC\AppScriptDependency;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this causes an psalm warning, since now OCP depends on OC. This wasn't found earlier since psalm was broken but it appear on #30508

I'm not sure if we can ignore this issue and just add it to the baseline or if we should fix it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine to add to the baseline from my perspective as it is only used in the method implementation and not exposed in any kinds.

@ChristophWurst ChristophWurst mentioned this pull request Nov 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@CarlSchwan CarlSchwan CarlSchwan left review comments

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@come-nc come-nc Awaiting requested review from come-nc

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish bug regression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Issues with dependency logic in Util::addScript

5 participants

@mejo- @juliusknorr @ChristophWurst @skjnldsv @CarlSchwan