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

protobuf: Update protobuf.git to 29.3 #62

Closed
wants to merge 1 commit into from
Closed

Conversation

flathubbot
Copy link
Contributor

🤖 This pull request was automatically generated by flathub-infra/flatpak-external-data-checker. Please open an issue if you have any questions or complaints. 🤖

@flathubbot
Copy link
Contributor Author

Started test build 172874

@tooomm
Copy link
Collaborator

tooomm commented Jan 9, 2025

@PunkPangolin FYI

@flathubbot
Copy link
Contributor Author

Build 172874 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/156000/io.github.Cockatrice.cockatrice.flatpakref

@PunkPangolin
Copy link
Contributor

@PunkPangolin FYI

Thanks! This is almost working as intended. Protobuf update works, and the JSON structure should only be reformatted this one time.
But it should not put the new version of protobuf into the metainfo file. I just raised an upstream issue for this.

"x-checker-data": {
"type": "git",
"tag-pattern": "^([\\d-]+Release-[\\d.]+)$",
"is-main-source": "true"
Copy link

Choose a reason for hiding this comment

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

Suggested change
"is-main-source": "true"
"is-main-source": true

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for having a look and helping super quickly!

Should not be a string value, yes.

Adressed in #63
I'm wondering if the tooling will pick the change up and updates this existing PR. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

The automation did create a new PR and somehow labels Cockatrice itself as update, too.

#64

@PunkPangolin
Copy link
Contributor

#64 looks good to me. The update to cockatrice just adds the related commit of the tag and does not actually update anything. This is working as intended and aims to prevent trouble in case of re-tagging.

@tooomm
Copy link
Collaborator

tooomm commented Jan 9, 2025

I figured that you included the commit hash for protobuf, but not Cockatrice in your PR. Hence the automation completes it. 👍

No change to the metainfo.xml in the other PR as well.

Is there some helpful docs and information next to the README in the flatpak-external-data-checker repo about x-checker-data in general?

@PunkPangolin
Copy link
Contributor

I figured that you included the commit hash for protobuf, but not Cockatrice in your PR. Hence the automation completes it. 👍

Exactly 👍 Forgot to add that.

Is there some helpful docs and information next to the README in the flatpak-external-data-checker repo about x-checker-data in general?

Not that I know of, sadly. What I know of it mostly stems from observation (and the README, of course). But it is a great tool that saves a lot of manual work.

@tooomm
Copy link
Collaborator

tooomm commented Jan 9, 2025

Couldn't find some more/other documentation either.

I moved the commit hash closer to its tag, same as protobuf. #68
Reordering the app json file and moving Cockatrice as main-source before protobuf for a more logical structure is failing for some reason though. #65 Any idea?


Really happy about your change, looking forward to the automated updates! :)
Will update the README accordingly as well and link to the tool.

@PunkPangolin
Copy link
Contributor

I moved the commit hash closer to its tag, same as protobuf.

This is also very nice for readability.

Reordering the app json file and moving Cockatrice as main-source before protobuf for a more logical structure is failing for some reason though. #65 Any idea?

What you see as logical structure is the wrong way around in the case of flatpak manifests ;)

Flatpak manifests get read (and built) from top to bottom (same inside nested modules). Therefore, the main module has to come last in the manifest. #65 fails because the modules are now built in the wrong order (Cockatrice needs protobuf, but protobuf is not built yes).

For some reason, this is not explicitly mentioned in the documentation, but you can find some examples there.

Really happy about your change, looking forward to the automated updates! :)

You're welcome!

@tooomm
Copy link
Collaborator

tooomm commented Jan 11, 2025

What you see as logical structure is the wrong way around in the case of flatpak manifests ;)

Flatpak manifests get read (and built) from top to bottom (same inside nested modules). Therefore, the main module has to come last in the manifest. #65 fails because the modules are now built in the wrong order (Cockatrice needs protobuf, but protobuf is not built yes).

For some reason, this is not explicitly mentioned in the documentation, but you can find some examples there.

That makes sense!

Thanks again for your contribution and sharing your knowledge. Much appreciated.

@tooomm tooomm closed this Jan 11, 2025
@tooomm tooomm deleted the update-19d6c9d branch January 11, 2025 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants