-
Notifications
You must be signed in to change notification settings - Fork 1.9k
recipe for libxml2 #999
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
recipe for libxml2 #999
Conversation
How about adding the full suite of libxml2, libxslt and lxml to the one PR? Since they're probably mostly going to be used together, this would make testing easier. |
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've made a few comments that you will want to address for this to work properly. Beyond that, have you tested it? Since this recipe has no dependencies, you should be able to build it once the recipe works.
|
||
|
||
class Libxml2Recipe(CythonRecipe): | ||
version = '2.9.4' |
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.
Presumably this version is much newer than the one the old toolchain used? It's hopefully fine and definitely should be targeted, but just to note it could be a source of problems.
|
||
class Libxml2Recipe(CythonRecipe): | ||
version = '2.9.4' | ||
url = 'ftp://xmlsoft.org/libxml2/libxml2-{version}.tar.gz' |
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'm not sure if p4a will work with ftp urls, have you tested 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.
It used to work with the old_toolchain ... so it would work with the new one. Wouldn't 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.
It definitely can work, I'm just not sure if ftp:// is handled properly right now. It shouldn't be a real problem.
|
||
try: | ||
sh.sed('runtest\$(EXEEXT) \\/ \\/', 'Makefile') | ||
except: |
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.
What are these try/except blocks for? If you expect a specific error, catch it explicitly, catching everything is bad style since you'll find it hard to debug problems that you didn't expect as they're swallowed the same way as the one you did anticipate. Since the old toolchain didn't expect any errors, you probably just don't want the try/except at all - if it fails, it's not for an expected reason so you can let p4a crash.
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.
@inclement You are right it would be difficult debug problems with all try and except statements.
I'll remove them.
env = super(Libxml2Recipe, self).get_recipe_env(arch) | ||
|
||
try: | ||
sh.sed('runtest\$(EXEEXT) \\/ \\/', 'Makefile') |
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 might not need to escape the $ character since this is a python string rather than shell code.
except: | ||
pass | ||
try: | ||
sh.make('-j', '$MAKE_JOBS') |
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.
$MAKE_JOBS
was an environment variable in the old toolchain, but this won't be picked up here, and in fact I don't think it exists any more. For now you could just set -j5
(possibly to be improved later).
from os.path import join | ||
|
||
|
||
class Libxml2Recipe(CythonRecipe): |
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.
This isn't a Cython module, so you'll want to just use Recipe
as the base class.
@inclement I'm working on libxslt, I'll send a pr when all are ready and also add BeautifulSoup4 recipe with it which was my real target. I'll also make changes to this recipe. |
patches = ['fix-dlopen.patch'] | ||
|
||
def should_build(self, arch): | ||
return not exists(join(self.get_build_container_dir(arch.arch),"libxslt/.libs/libxslt.a")) |
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 you can use self.get_build_dir(arch.arch)
and remove the libxslt
folder from the path - this should work the same, but will also work if the extracted libxslt folder is something unexpected. This is a small thing though, it probably isn't important to having the recipe actually work.
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.
@inclement Yes, it can cause problems. I have changed it.
@yaki29 This all looks good on a quick check, nice work. But, have you tried it, does it work? |
"Robots and Screen Scraping: You may not use data mining, robots, screen scraping, or similar data gathering and extraction tools on this site, except with our express written consent as noted below. " http://www.imdb.com/conditions We could use a site which does not forbid scraping. There are some other cleanups to do in the example, i'm not sure what is happening with those globals, pep8 method names etc. Though it's also not clear if the example is even necessary, there is nothing kivy/p4a specific needed for bs4 to work in terms of code. |
++ to @dessant's comments, although I'll note that I asked for this example as these are very useful for testing p4a - I don't know anything about beautifulsoup or its compiled dependencies, so it would be difficult to test without an example. |
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.
testapps/BeautifulSoup4/main.py
Outdated
|
||
self.Pop(movie,imdb,actor,director,writer,url) | ||
|
||
except Exception: |
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.
When catching exceptions, work out what exception you expect and catch that one specifically. Catching Exception itself will make the code ignore just about anything, which can hide all manner of problems.
testapps/BeautifulSoup4/main.py
Outdated
on_press: root.detail(name.text) | ||
|
||
''') | ||
global writer |
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 don't need to declare these as global.
testapps/BeautifulSoup4/main.py
Outdated
popup = Popup(title="ERROR",content=Label(text="Bro, Check your movie name"),size_hint=(0.8,0.8)) | ||
popup.open() | ||
|
||
def Pop(self,movie,imdb,actor,director,writer,url): |
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.
It would be preferable to use kv to set up these labels.
@dessant @inclement I didn't know that imdb doesn't let you scrap it's data. Sorry for that. I'll correct those mistakes pointed by you. |
@yaki29 I think you could use OpenLibrary or e.g. a static website that has no big database in the background. |
Scraping the content of the |
Thanks @dessant @KeyWeeUsr . I was about to commit the changes. I was little busy for last two days because of some college work. I'll send a commit. |
pass | ||
sh.sed('runtest$(EXEEXT) \/ \/', 'Makefile') | ||
sh.sed('testrecurse$(EXEEXT)$//', 'Makefile') | ||
sh.make('-j', '5') |
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.
These lines should be placed in the build_arch
method, and you need to navigate to the right directory, something like
def build_arch(self, arch):
with current_directory(self.get_build_dir(arch)):
sh.sed(....)
import sh | ||
from os.path import join | ||
|
||
class BSoup4Recipe(PythonRecipe): |
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.
The fact that you need to set the compiler stuff seems to suggest that this is not a pure python recipe, so maybe you need to use CythonRecipe
or CompiledComponentsPythonRecipe
instead.
testapps/BeautifulSoup4/main.py
Outdated
class PopupScreen(BoxLayout): | ||
def error(self): | ||
popup = Popup(title="ERROR", | ||
content=Label(text="Check Your Network Connection"), |
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.
If you must catch exceptions in this example, please show the exception message instead of assuming what the error is.
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.
@dessant Will do the correction.
What's the current status of this, is it working for you? |
|
||
sh.sed('runtest$(EXEEXT) \/ \/', 'Makefile') | ||
sh.sed('testrecurse$(EXEEXT)$//', 'Makefile') |
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.
@inclement it is having error here(with this recipe alone others are working fine ( I have not tested bs4 recipe as it depends on this one)).
@inclement I'll look into it and try to make it work. |
Hi, is there any idea when this could be merged? I use lxml and want to migrate to the new toolchain to solve the problem with the old OpenSSL (on Google Play). Thanks for an update... |
@FilipDem I'm right now working on kvCompiler for gsoc 2017. But I assure you I'll spare some time and complete it. |
Hi Yaki29, |
Is there any idea when the lxml will be available with the new toolchain? I am using this intensively and porting to cElementTree give really problems. Thanks for the info. |
@FilipDem This isn't something I'm likely to attack directly any time soon, so it probably won't get into p4a unless someone else submits a PR. |
Thanks for the information, Inclement. Is there any alternative? I tried using cElementTree, but it was not successful. |
@FilipDem I'm afraid I'm not familiar with these libraries and their alternatives. If you are able, it would help to provide a simple runnable example using lxml functionality, so that anyone who does try to make a recipe has something to test to check if it's actually working. |
I just added #1166 which should implement an |
Hello everybody, a PR based on #1166 has been merged, so this one can now be closed (ping @AndreMiras). Thanks to everybody who worked on that. |
Fixed in #1428 thanks for the heads-up |
This is a recipe for libxml2. If there are still some changes to be done I would be happy to do them.