-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
base: develop
Are you sure you want to change the base?
Conversation
allow tweaking it and using the tweaked version in future builds.
There was a problem hiding this 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
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? |
I would love to be able to extend the default template file with arbitrary blocks for my project |
allow tweaking it and using the tweaked version in future builds.