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

AppStream metainfo fixes #4205

Merged
merged 5 commits into from
Sep 8, 2021
Merged

AppStream metainfo fixes #4205

merged 5 commits into from
Sep 8, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Aug 15, 2021

No description provided.

@Be-ing Be-ing changed the base branch from main to 2.3 August 15, 2021 12:48
@github-actions github-actions bot added the build label Aug 15, 2021
@uklotzde
Copy link
Contributor

This change might break existing packaging recipes and requires a note for maintainers.

@uklotzde uklotzde added this to the 2.3.1 milestone Aug 15, 2021
@uklotzde uklotzde added packaging/installer changelog This PR should be included in the changelog labels Aug 15, 2021
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 15, 2021

I'll add a note to packagers in the CHANGELOG after both this and #4204 are merged.

res/linux/org.mixxx.Mixxx.metainfo.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Merging the generated org.mixxx.Mixxx.metainfo.xml might become difficult. Changes in the release history need to be ignored and re-generated in the target branch. Did you consider this?

@Holzhaus
Copy link
Member

I think it should be straightforward:

  1. Merge conflict in the metainfo.xml file
  2. Instead of running git mergetool, just add the metainfo file including the conflict markers to the staging area
  3. Now run pre-commit run metainfo (all changes are staged)
  4. The hook regenerates the metainfo.xml
  5. git add the changes and commit

@uklotzde
Copy link
Contributor

How about adding a comment before the <releases> start tag that explains that the contents are generated by pre-commit run metainfo? This is not obvious and not documented.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 21, 2021

How about adding a comment before the start tag that explains that the contents are generated by pre-commit run metainfo? This is not obvious and not documented.

Good idea, done.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 21, 2021

Something weird is going on with the new pre-commit hook. It passed when I made the last commit, but now GitHub Actions is not passing. I downloaded and applied the patch from GitHub Actions, then tried to commit but when the hook ran again locally it canceled out the patch??

@Holzhaus
Copy link
Member

Did you rebase or something. The changes look legit, not some miniscule formatting issues. So I suppose there is a difference in the changelog versions.

@uklotzde
Copy link
Contributor

Locally the metainfo hook for this branch passes. We cannot merge before the GitHub issues are fixed.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 24, 2021

I removed the commit with the manual changes and reran the script. Now pre-commit passes.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 8, 2021

Please don't let this linger any longer. I am tired of having to rebase and rerun the script.

for section in re.split("^## ", content.strip(), flags=re.MULTILINE)[1:]:
version, sep, description_md = section.partition("\n")
matchobj = re.match(
r"\[(?P<number>\d\.\d\.\d)\]\([^\)]+\)"
Copy link
Member

Choose a reason for hiding this comment

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

I struggle to understand the [^\)]+ part.

Copy link
Member

Choose a reason for hiding this comment

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

I recommend regex101.com:
Screenshot from 2021-09-08 17-08-07

It's supposed to match any char except ) one or more times (for the milestone URL):
Screenshot from 2021-09-08 17-13-14

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Ok for me.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 8, 2021

Merge?

@Holzhaus Holzhaus merged commit 989f7c8 into mixxxdj:2.3 Sep 8, 2021
@Be-ing Be-ing deleted the metainfo branch September 8, 2021 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build changelog This PR should be included in the changelog code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants