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

allow dumping and loading the manifest file #1439

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tshirtman
Copy link
Member

allow tweaking it and using the tweaked version in future builds.

allow tweaking it and using the tweaked version in future builds.
Copy link
Member

@inclement inclement left a comment

Choose a reason for hiding this comment

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

Didn't see this before, sorry, not sure how I missed it.
I've nothing against it, but I think it should give some kind of warning that this completely skips the normal manifest process. It would probably be easy to accidentally make a manifest that doesn't work for whatever reason. For normal usage, I prefer the direction of making the template more flexible.

android_api=android_api,
url_scheme=url_scheme)

if args.save_manifest:
Copy link
Member

Choose a reason for hiding this comment

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

I think that save_manifest should happen regardless of whether load_manifest was set. Even if that isn't useful (i.e. saving the one the user provided in the first place), at least it's consistent. Or they could be marked as incompatible arguments instead.

Copy link
Member Author

@tshirtman tshirtman Dec 8, 2018

Choose a reason for hiding this comment

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

You mean make the render even if we overwrite it just after (moving the if/shutil.copy part after the current else branch) ? that seems a bit wasteful, and i'm not sure how it helps with consistency, but i'm fine with it if you prefer.

ap.add_argument('--save-manifest', dest='save_manifest', default='',
help="If set to a path, the created manifest will be saved to this path.")
ap.add_argument('--load-manifest', dest='load_manifest', default='',
help="If set to a path, the manifest will be loaded from this path.")
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the paths will be messed up at this point, since the script is run from a directory that is different to the one the user ran from, so e.g. if they wrote './mymanifest.xml' that path won't make sense any more. I think there is some handling of this issue elsewhere in the p4a arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a good point, we should probably document that it should be an absolute path.

@tshirtman
Copy link
Member Author

tshirtman commented Dec 8, 2018

About making the current way more flexible, i do agree, but there always will be things that will be hard to support without more or less giving a tree like structure instead of just flat options, so short of doing a full mapping of the xml file through json/yaml or whatever (with very dubious benefits imho) giving a direct access to the file is more or less necessary.

@inclement inclement changed the base branch from master to develop June 6, 2019 20:41
@tshirtman
Copy link
Member Author

tshirtman commented Apr 9, 2021

would an xml "merge" be a better way, allowing to add or replace part of the xml file with determined sections, be a better plan? or even, simple diff/patch logic so the user can simply apply their change?
Maybe giving a python access to the xml tree through a hook system, allowing one to add/remove/update the nodes?
Because exposing all the possible xml options seems like an endless churn to me.

@Fak3
Copy link

Fak3 commented Apr 10, 2021

I would love to be able to extend the default template file with arbitrary blocks for my project

@Fak3
Copy link

Fak3 commented Apr 10, 2021

Related to #1922 #2200

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.

None yet

3 participants