Skip to content

recipe for libxml2 #999

Closed
yaki29 wants to merge 7 commits into
kivy:masterfrom
yaki29:master
Closed

recipe for libxml2 #999
yaki29 wants to merge 7 commits into
kivy:masterfrom
yaki29:master

Conversation

@yaki29

@yaki29 yaki29 commented Jan 27, 2017

Copy link
Copy Markdown
Contributor

This is a recipe for libxml2. If there are still some changes to be done I would be happy to do them.

@inclement

Copy link
Copy Markdown
Member

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.

@inclement inclement left a comment

Copy link
Copy Markdown
Member

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'

Copy link
Copy Markdown
Member

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'

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Member

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:

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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')

Copy link
Copy Markdown
Member

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')

Copy link
Copy Markdown
Member

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):

Copy link
Copy Markdown
Member

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.

@yaki29

yaki29 commented Jan 28, 2017

Copy link
Copy Markdown
Contributor Author

@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"))

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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.

@inclement

Copy link
Copy Markdown
Member

@yaki29 This all looks good on a quick check, nice work. But, have you tried it, does it work?

@dessant

dessant commented Feb 1, 2017

Copy link
Copy Markdown
Contributor

"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.

@inclement

Copy link
Copy Markdown
Member

++ 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.

@inclement inclement left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've added a few comments on specific parts of the testapps. As @dessant noted, please also try to follow pep8.

Comment thread testapps/BeautifulSoup4/main.py Outdated

self.Pop(movie,imdb,actor,director,writer,url)

except Exception:

Copy link
Copy Markdown
Member

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.

Comment thread testapps/BeautifulSoup4/main.py Outdated
on_press: root.detail(name.text)

''')
global writer

Copy link
Copy Markdown
Member

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.

Comment thread 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):

Copy link
Copy Markdown
Member

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.

@yaki29

yaki29 commented Feb 1, 2017

Copy link
Copy Markdown
Contributor Author

@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.

@KeyWeeUsr

Copy link
Copy Markdown
Contributor

@yaki29 I think you could use OpenLibrary or e.g. a static website that has no big database in the background.

@dessant

dessant commented Feb 4, 2017

Copy link
Copy Markdown
Contributor

Scraping the content of the h1 tag from https://httpbin.org/html could be enough, and keeping the example simple.

@yaki29

yaki29 commented Feb 4, 2017

Copy link
Copy Markdown
Contributor Author

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')

Copy link
Copy Markdown
Member

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):

Copy link
Copy Markdown
Member

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.

Comment thread testapps/BeautifulSoup4/main.py Outdated
class PopupScreen(BoxLayout):
def error(self):
popup = Popup(title="ERROR",
content=Label(text="Check Your Network Connection"),

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

@inclement

Copy link
Copy Markdown
Member

What's the current status of this, is it working for you?

return env

sh.sed('runtest$(EXEEXT) \/ \/', 'Makefile')
sh.sed('testrecurse$(EXEEXT)$//', 'Makefile')

Copy link
Copy Markdown
Contributor Author

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)).

@yaki29

yaki29 commented Mar 5, 2017

Copy link
Copy Markdown
Contributor Author

@inclement I'll look into it and try to make it work.

@FilipDem

Copy link
Copy Markdown

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...

@yaki29

yaki29 commented Mar 18, 2017

Copy link
Copy Markdown
Contributor Author

@FilipDem I'm right now working on kvCompiler for gsoc 2017. But I assure you I'll spare some time and complete it.

@FilipDem

Copy link
Copy Markdown

Hi Yaki29,
I cannot find yet the lxml recipe in the master branch. Is this normal?
Thanks for your help!

@FilipDem

Copy link
Copy Markdown

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.

@inclement

Copy link
Copy Markdown
Member

@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.

@FilipDem

Copy link
Copy Markdown

Thanks for the information, Inclement. Is there any alternative? I tried using cElementTree, but it was not successful.

@inclement

Copy link
Copy Markdown
Member

@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.

@eacpereira

Copy link
Copy Markdown

I just added #1166 which should implement an lxml recipe. Needs testing, tho.

@goffi-contrib

Copy link
Copy Markdown
Contributor

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.

@AndreMiras

Copy link
Copy Markdown
Member

Fixed in #1428 thanks for the heads-up

@AndreMiras AndreMiras closed this Nov 5, 2018
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.

8 participants