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

Adopt AppStream #6056

Merged
merged 2 commits into from
Jun 9, 2021
Merged

Adopt AppStream #6056

merged 2 commits into from
Jun 9, 2021

Conversation

overflw
Copy link
Contributor

@overflw overflw commented Apr 18, 2021

With those small changes I hope to make tribler compatible with the AppStream standard. I couldn't test the changes yet, because tribler won't build for me at the moment.

@tribler-ci
Copy link
Contributor

Can one of the admins verify this patch?

@devos50
Copy link
Contributor

devos50 commented Apr 19, 2021

ok to test

<content_attribute id="social-info">mild</content_attribute>
</content_rating>
<releases>
<release version="7.8.0" date="2021-02-15"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

We refrain from hard-coding versions in our files. Please have a look at this script that is executed before packaging Tribler 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@overflw this is a way to satisfy this request for changes:

  1. Remove the last three lines from this file (line 47, line 48 and line 49) and leave the last line blank.

  2. Paste the following code into the __main__ section of that script:

logger.info('Writing AppStream version.')
with open(path.join('build', 'debian', 'tribler', 'usr', 'share', 'metainfo',
                    'org.tribler.Tribler.metainfo.xml'), 'a') as f:
    f.write(f'    <release version="{version_id}" date="{build_date}"/>'
            f'\n  </releases>\n</component>')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey (: thanks for the hints! I wrote a xml parsing based addendum to update_version_from_git.py:

    logger.info('Writing AppStream version info.')
    releaseDate = time.strftime("%Y-%m-%d", time.localtime())
    attrib = {'version': f'{version_id}', 'date':f'{releaseDate}'}

    tree = xml.parse(path.join('build', 'debian', 'tribler', 'usr', 'share', 'metainfo',
                        'org.tribler.Tribler.metainfo.xml'))
    xmlRoot = tree.getroot()
    releases = xmlRoot.find('releases')
    release = xml.SubElement(releases, 'release', attrib)
    tree.write(path.join('build', 'debian', 'tribler', 'usr', 'share', 'metainfo',
                        'org.tribler.Tribler.metainfo.xml'))

@ghost
Copy link

ghost commented Apr 19, 2021

Congratulations 🎉. DeepCode analyzed your code in 3.971 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

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.

Thanks for your changes! Could you look at/implement the suggestion raised by DeepCode? Also, please satisfy the flake8 check by making sure the import order is according to the standards 👍

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.

Thanks for the changes! I just installed defusedxml on the machine that runs the flake8 checker. @xoriole do you think it is fine to add this as a build dependency or do you prefer to use the 'normal' XML parsing tools?

@overflw
Copy link
Contributor Author

overflw commented Apr 20, 2021

Thanks for the changes! I just installed defusedxml on the machine that runs the flake8 checker. @xoriole do you think it is fine to add this as a build dependency or do you prefer to use the 'normal' XML parsing tools?

Tbh, defusdxml is not really necessary here, since we only parse our own metainfo.xml. But it won't hurt I guess.

@xoriole
Copy link
Contributor

xoriole commented Apr 21, 2021

@devos50 @overflw I'm fine with having it as a build dependency for Linux. I'd prefer it to be conditional import and the code for xml parsing and update to be under Linux section (Line 60).

@devos50
Copy link
Contributor

devos50 commented Apr 23, 2021

retest this please

1 similar comment
@xoriole
Copy link
Contributor

xoriole commented Apr 23, 2021

retest this please

xoriole
xoriole previously approved these changes Apr 23, 2021
@overflw
Copy link
Contributor Author

overflw commented Apr 27, 2021

Hey, sorry for dismissing the review - I haven't seen it before pushing.. what can I do to close this pull request?

@devos50
Copy link
Contributor

devos50 commented Apr 27, 2021

@overflw could you rebase and please squash your commit history into one commit (and remove the merge commits)?

@sonarqubecloud
Copy link

Kudos, SonarCloud 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

@overflw
Copy link
Contributor Author

overflw commented Apr 28, 2021

@overflw could you rebase and please squash your commit history into one commit (and remove the merge commits)?

Should be done, I hope.

@qstokkink
Copy link
Contributor

qstokkink commented Apr 29, 2021

Failure on Mac is due to #6042:

Error Message

assert 59852 == 59850   +59852   -59850

Stacktrace

def test_get_first_free_port():
        # target port is free
        port = random.randint(50000, 60000)
>       assert get_first_free_port(start=port) == port
E       assert 59852 == 59850
E         +59852
E         -59850

src/tribler-core/tribler_core/tests/test_network_utils.py:56: AssertionError

This is a random failure, so retesting this PR should fix it.

@qstokkink
Copy link
Contributor

retest this please

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 9, 2021

Kudos, SonarCloud 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.0% 0.0% Duplication

@xoriole
Copy link
Contributor

xoriole commented Jun 9, 2021

retest this please

@xoriole xoriole merged commit 9347a00 into Tribler:main Jun 9, 2021
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.

5 participants