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

Run Upgrader in a GUI thread #6559

Merged
merged 4 commits into from
Nov 22, 2021
Merged

Conversation

ichorid
Copy link
Contributor

@ichorid ichorid commented Nov 16, 2021

Partially fixes #6525

This one moves some of the functions of Core Manager into the new Upgrade Manager class. The upgrader is now run in a GUI QThread for simplicity. Also, this changes run_tribler.py to use arguments instead of environment variables to indicate the running mode (Core vs GUI vs Core Test Mode). Arguments are safer and unambiguous.

The main point of this PR is to stop sending event notifications from the Core from upgrader and thus pave the road for simplification of Tribler startup.

Upgrade Manager is briefly run during GUI tests, quitting when not able to find an existing Tribler state dir. This is not a real test, but at least it covers all the relevant lines. The downside is that when running single tests from the GUI suite waiting for additional time could be a bit annoying.

Simplification of GUI tests uncovered some hidden race conditions, which required minor fixes.

@ichorid
Copy link
Contributor Author

ichorid commented Nov 16, 2021

retest this please

@ichorid ichorid changed the title Move upgrader into a separate process Move upgrader out of the Core Nov 17, 2021
@ichorid ichorid changed the title Move upgrader out of the Core Run Upgrader in a GUI thread Nov 17, 2021
@ichorid
Copy link
Contributor Author

ichorid commented Nov 17, 2021

retest this please

1 similar comment
@ichorid
Copy link
Contributor Author

ichorid commented Nov 17, 2021

retest this please

@ichorid ichorid force-pushed the refactor/extract_upgrader branch 2 times, most recently from ac128e3 to 8f62777 Compare November 17, 2021 20:19
@ichorid ichorid marked this pull request as ready for review November 17, 2021 20:25
@ichorid ichorid requested review from a team, kozlovsky, drew2a and devos50 and removed request for a team November 17, 2021 20:25
Copy link
Contributor

@drew2a drew2a left a comment

Choose a reason for hiding this comment

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

This PR definitely looks like an improvement.

I've not reviewed the logic, because I don't understand it. I Hope @kozlovsky or @devos50 can do it properly.

I left some comments.
Also, pylint and coverage checks should be satisfied.

src/run_tribler.py Show resolved Hide resolved
src/run_tribler.py Show resolved Hide resolved
src/tribler-core/run_tribler_upgrader.py Outdated Show resolved Hide resolved
src/tribler-core/tribler_core/tests/test_start_core.py Outdated Show resolved Hide resolved
src/tribler-gui/tribler_gui/tests/test_gui.py Outdated Show resolved Hide resolved
@ichorid
Copy link
Contributor Author

ichorid commented Nov 18, 2021

@drew2a , the remaining lines will be very hard to cover. In fact, this PR increases the coverage - the lines that are shown as non-covered were never covered before (they just count as new lines, because these were extracted from CoreManager).

src/run_tribler.py Outdated Show resolved Hide resolved
src/tribler-core/tribler_core/start_core.py Show resolved Hide resolved
src/tribler-gui/tribler_gui/tests/test_gui.py Outdated Show resolved Hide resolved
src/tribler-gui/tribler_gui/upgrade_manager.py Outdated Show resolved Hide resolved
kozlovsky
kozlovsky previously approved these changes Nov 19, 2021
@ichorid ichorid force-pushed the refactor/extract_upgrader branch 6 times, most recently from e46e571 to 7bef4e4 Compare November 19, 2021 15:57
@ichorid
Copy link
Contributor Author

ichorid commented Nov 19, 2021

retest this please

@ichorid ichorid force-pushed the refactor/extract_upgrader branch 2 times, most recently from d4625b3 to 6fa367b Compare November 22, 2021 18:51
Copy link
Contributor

@devos50 devos50 left a comment

Choose a reason for hiding this comment

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

Looks good - all my feedback has been implemented. I just have one suggestion left.

Before merging, please make sure the pylint errors are fixed 👍

src/run_tribler.py Show resolved Hide resolved
src/run_tribler.py Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Nov 22, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

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

Successfully merging this pull request may close these issues.

4 participants