-
Notifications
You must be signed in to change notification settings - Fork 242
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
Added the ability to pass arguments to as a string #726
base: master
Are you sure you want to change the base?
Conversation
Previously, if you read dependencies from a file and passed `"$requirements"` from toolchain build, then the script accepted dependencies with one line (`['kivy python3']`), now this is fixed
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.
Left a couple of comments.
kivy_ios/toolchain.py
Outdated
@@ -1511,7 +1512,8 @@ def pip3(self): | |||
self.pip() | |||
|
|||
def pip(self): | |||
_pip(sys.argv[2:]) | |||
args = ' '.join(sys.argv[2:]).replace(',', '').replace(' ', ' ').split() |
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.
_pip
is a "redirector" to an actual pip, so the arguments should be pip-compatible.
Why not using a requirements.txt
file here?
toolchain pip install -r app-req.txt
, would just work 😃
(Maybe we want to document it?)
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.
hm, yes, it must be documented
kivy_ios/toolchain.py
Outdated
@@ -1365,7 +1365,8 @@ def build(self): | |||
if ctx.use_pbzip2: | |||
logger.info("Using pbzip2 to decompress bzip2 data") | |||
|
|||
build_recipes(args.recipe, ctx) | |||
recipe = ' '.join(args.recipe).replace(',', '').replace(' ', ' ').split() |
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.
Easy and effective, but looks quite hacky and I don't feel super-confident about merging it. (And should be handled on the argparser side instead)
How about an argument -r
or --requirement
which parses a requirements.txt
file as pip
does ?
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.
Sounds good, why not, I'll try to implement
Look, maybe we can change the argument parser, now it works like this: |
Previously, if you read dependencies from a file and passed
"$requirements"
from toolchain build, then the script accepted dependencies with one line (['kivy python3']
), now this is fixedExample:
we have file requirements.txt with (python3, kivy, openssl pillow)
although we have double spaces and commas in the file, the libraries are read and assembled correctly
test_req.txt