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

Fixes 1-Click release for 7.11 #6568

Merged
merged 12 commits into from
Nov 20, 2021
Merged

Conversation

kozlovsky
Copy link
Contributor

@kozlovsky kozlovsky commented Nov 19, 2021

This PR together with #6545 contains fixes for various problems with Tribler builds.
With this fixes, 1-Click release Jenkins job is able to assemble executables and test them with application tester

@kozlovsky kozlovsky requested review from a team, drew2a and xoriole and removed request for a team November 19, 2021 16:35
@kozlovsky kozlovsky marked this pull request as draft November 19, 2021 16:56
@kozlovsky kozlovsky force-pushed the release/7.11 branch 3 times, most recently from 9dd50a6 to dd410a3 Compare November 19, 2021 18:14
@sonarqubecloud
Copy link

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
No Duplication information No Duplication information

@@ -46,7 +46,7 @@ REM Arno: When adding files here, make sure tribler.nsi actually
REM packs them in the installer .EXE

python3 -m pip install --upgrade --no-deps --force-reinstall pydantic --no-binary pydantic
python3 -m pip install --upgrade --no-deps --force-reinstall typing_extensions --no-binary typing_extensions
python3 -m pip install --upgrade --no-deps --force-reinstall typing_extensions==3.10.0.2 --no-binary typing_extensions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without --no-binary PyInstaller does not include this module into the Tribler exe file on Windows
The latest version of typing_extension (4.0.0) is currently not working as well.
After #6534 we can move it into requirements with --no-binary specification

@@ -89,8 +89,14 @@ def should_kill_other_tribler_instances(root_state_dir):
if current_pid != old_pid and old_pid > 0:
# If the old process is a zombie, simply kill it and restart Tribler
old_process = psutil.Process(old_pid)
logger.info(f'Old process status: {old_process.status()}')
if old_process.status() == psutil.STATUS_ZOMBIE:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes Tribler crashes on psutil.NoSuchProcess exception when reading old_process.status()

if trace_logger:
trace_logger.close()

process_checker.remove_lock_file()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We must always remove the lock file on Tribler crash to re-run the application tester reliably, so try/finally block is necessary here.

Copy link
Contributor Author

@kozlovsky kozlovsky Nov 19, 2021

Choose a reason for hiding this comment

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

Our lock file mechanics are broken, as it was built on the assumption that OS does not reuse PIDs before system restart. This is not true, Windows especially reuses process IDs quickly, and that leads to many non-obvious bugs (like, Tribler is trying to kill some innocent unrelated process, assuming this is the previous Tribler core). I will open a separate issue for that.

@@ -64,7 +64,12 @@ def on_core_read_ready(self):
if b'Traceback' in raw_output:
self.core_traceback = decoded_output
self.core_traceback_timestamp = int(round(time.time() * 1000))
print(decoded_output.strip()) # noqa: T001
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes Tribler crashes when trying to write data to stdout during the shutdown

@kozlovsky kozlovsky marked this pull request as ready for review November 19, 2021 19:09
@kozlovsky kozlovsky requested a review from devos50 November 19, 2021 19:09
@kozlovsky kozlovsky changed the title Release/7.11 Fixes 1-Click release for 7.11 Nov 19, 2021
# Flush all the logs to make sure it is written to file before it exits
for handler in logging.getLogger().handlers:
handler.flush()

SentryReporter.global_strategy = SentryStrategy.SEND_SUPPRESSED
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this Sentry suppression, it sometimes happens that an error takes place during the Tribler shutdown. Then Sentry tries to display the dialog window, but the Qt state is already partially destructed, which leads to mysterious crashes.

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.

Great set of changes! 👍

'pyqtgraph',
'pyqtgraph.graphicsItems.PlotItem.plotConfigTemplate_pyqt5',
'pyqtgraph.graphicsItems.ViewBox.axisCtrlTemplate_pyqt5',
'pyqtgraph.imageview.ImageViewTemplate_pyqt5',
'PyQt5.QtTest',
'requests',
'scrypt', '_scrypt',
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe pyaes, sqlalchemy and script are not required anymore. These were dependencies for bitcoinlib (yes, we used to have a Bitcoin wallet in Tribler 😁 ) but we remove that library and we apparently forgot to remove them from tribler.spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know! I'll remove them in a separate PR after additional testing

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.

2 participants