-
Notifications
You must be signed in to change notification settings - Fork 241
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
Cleanup + Add test in CI for cookiecutter related things #454
Conversation
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.
Feels good!
So these tools/externals/ were definitely 100% replaceable by the actual pypi modules?
I would simply take a look at the git log of some of them to see if they were maintained to add features used in kivy-ios.
Also having someone else using kivy-ios to cross check before merging would be great
maybe it could have been a PR against https://github.com/albert-zhang/gen_xcassets |
I have really little clue about macOS so I can't tell. But the code base doesn't look the same so at least it wasn't heavily borrowed from there. I would say to keep it simple for now I would simply undelete the |
true, maybe it's not the correct one, i agree it's simpler to keep this one for now. |
misl6 just gave me a heads up on Discord and didn't actually delete it 😅 |
On checking out the branch:
|
So. I'm guessing we need to install those requirements. One issue I have with that is that I've been unable to run the toolchain successfully in a venv. I can only get a successful build using the python version installed via brew. So depending on libraries in the native python install is...a pity. It would also mean we would need to change installation instructions. |
Even when creating a venv, I get this err. I think there was a reason that these external tools where included in the repo.. |
The |
@AndreMiras I know it's added to requirements. Please see my comments above. But as I mentioned, the issues are:
To be honest, I'd rather live with the ugliness of external/tools that depending on requirements being installed to the systems python. I'd only be happy if a venv could be used.... |
OK let's keep this pending until we fix the venv then |
As agreed with Zen-Code let's fix the venv first
200824a
to
6b4e1a3
Compare
6b4e1a3
to
ae20a72
Compare
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.
LGTM, I'll play with it soon.
Ideally we should also remove the second pip install sh
from the venv test since it's now part of of the requirements.txt
file. It's rather minor and CI builds for ages so I guess it's OK we don't address now.
I will give it a try now see if I can build/run a project without having any of the deps in the system but only in the venv
ae20a72
to
b8f2fd0
Compare
So far so good, here's what I've done to test it.
Then I've created a completely new venv to build recipes:
It built successfully. I'm now about to try on device and merge if it works OK |
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.
App ran fine on device simulator nice work!
Thanks for addressing the comments and squashing the commits 🍻
tools/external