search

mauroc / squiddio_pi

squiddio_pi
3 stars 13 forks source link

update to remove plugin name from file, make more generic #105

Closed jongough closed 4 years ago

leamas commented 4 years ago

Right :+1:

However, you can do a push -f in situations like this. This will effectively rewrite the PR, while keeping the discussion intact.

leamas commented 4 years ago

Copying my last comment from #104:

I am trying to make the process easier for other plugin developers so that they don't have to go through such a steep learning curve.

Which is fine, really. There is a small gotcha, though: Making things generic adds complexity. That is not to say that it is wrong in all (or this) situation. It's just not simple.

Here is a basic trade-off: keeping it focused on squiddio, or making the code a great starting point for other devs. I cannot judge what is the proper way here, it's just not my decision to make.

In the end, IMHO this is something @mauroc should decide, probably best in a separate bug like "Make squiddio a generic starting point for other devs" or so.

However, there is another string to pull. One might certainly file a bug like "Configuration spread into too many places". And make changes with this in focus. Having most of the configuration in CMakeLists.txt is of course big advantage (even though I doubt that flatpak/*yaml should be handled this way). There are some steps in this direction in #101.

leamas commented 4 years ago

BTW: Since this is about the flatpak build: Have you got a pointer to a successful build of this for flatpak?

Yes, that only the appveyor builds are done to PRs is a pain :(

leamas commented 4 years ago

Oops, it doesn't seem to build: https://circleci.com/workflow-run/3fbe69d5-ab4f-46e6-a099-07d926caf87d

This after cherry-picking a6ba6b on top of #101, so it's not identical. But still...

jongough commented 4 years ago

Missed updating the yaml file to put the correct values in. Also had to uncomment the 'commit' line in the yaml file. Not really sure why the commit is there as it seems a bit 'targeted' rather than free to pick the latest update to the master branch.

leamas commented 4 years ago

Until #106 is resolved I suggest that you provide a link to a build of this PR. Should make @mauroc more comfortable if he wants to merge it.

leamas commented 4 years ago

This PR illustrates the basic trade-off between keeping it simple and being generic. It basically makes it possible to change plugin name in a simple way. The cost is the added complexity with a yaml file generated from a template.

I'm not the one who should judge if this is the right thing to do. I'm just trying to illustrate the trade-off so that others can make their decisions in best possible way.

jongough commented 4 years ago

OK

rgleason commented 4 years ago

That commit hash being required in the file is a problem, in my opinion.

leamas commented 4 years ago

First step is to try to understand the flatpak build process, which is very different. One could argue it should be something else, but basically to no avail. At least here.

Given the build process design criterias it actually does the Right Thing. In particular, it can rebuild a flatpak application just given the yaml file..

To handle things like this one need to respect the upstream releases. Using a commit is not the most natural thing, the normal stuff is using a tag. If there are changes to be applied against the tag, one need to create one or more patches. It's standard practice when doing downstream packaging (which is what a flatpak is). git, in particular git format patch, is extremely helpful in this context.

An introduction to the yaml file, together with links to reference docs, is at http://docs.flatpak.org/en/latest/manifests.html

leamas commented 4 years ago

That commit hash being required in the file is a problem, in my opinion.

I shouldn't have replied, should I? After all, is this related to this PR in any way?!

If you want to raise this as a problem, it's probably best done in a separate issue.