963176 - Solve dependency situation for external pefile package on pypi

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This bug has been created as a follow-up for bug 960084 comment 19, which is about:

> > The pefile module came in via bug 795288, which helps us to read out the
> > PEHeader of Windows EXE files. What would you suggest to do here?
> 
> We'll either need to:
> A) check pefile into the tree under /python if the license permits
> B) not include it in setup.py and only use it if it's detected on the system
> C) re-implement the parts of pefile we care about within mozbase itself.

A solution has to be picked here, so we can actually move mozbase to mozilla-central on bug 949600. So it's blocking.

Jarek, would you mind to check the license of the package, and what it would mean for A) and C).

Comment 1

[Jarek Śmiejczak [:jotes]]

:whimboo
License on project site states it's MIT License. However, COPYING file:
https://code.google.com/p/pefile/source/browse/trunk/COPYING?spec=svn140&r=139
Looks like it allows source redistribution etc.

If that's required, i can ask author directly.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Gerv, can you please give us feedback if copying the files is ok, or what's best regarding the MIT license? Thanks.

Comment 3

[Gervase Markham [:gerv]]

That's not the MIT license, that's a 3-clause BSD license. It's fine to bring code under that license in to the Mozilla tree. I assume we are not shipping it with Firefox or any other product? If that's right, then all you have to do is make sure the file is appropriately labelled. If it already has its own license header, fine. Otherwise, put it in its own directory with a copy of the LICENSE file.

Gerv

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Gervase Markham [:gerv] from comment #3)
> bring code under that license in to the Mozilla tree. I assume we are not
> shipping it with Firefox or any other product? If that's right, then all you

We would ship this code with a mozbase package to pypi, not such which one it will be but it might be mozinstall. See https://pypi.python.org/pypi/mozInstall. Would that be a problem?

Comment 5

[Andrew Halberstadt [:ahal]]

No, we aren't shipping it with mozinstall, mozinstall just requires it as a dependency. And no this doesn't get shipped with Firefox or any other applications either. So lets go ahead and land this under /python, make sure the LICENSE file is in there and add the module to this list:
http://dxr.mozilla.org/mozilla-central/source/build/mach_bootstrap.py#27

Comment 6

[Andrew Halberstadt [:ahal]]

Also to this file:
http://dxr.mozilla.org/mozilla-central/source/build/virtualenv_packages.txt

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andrew, so that means you are ok with option A) then. Sounds fine then.

Comment 8

[Andrew Halberstadt [:ahal]]

Attachment: 0001-Bug-963176-mozinstall-should-gracefully-fallback-if-.patch

I thought about this a bit and it seems kind of yucky to check an external module into the tree when nothing in-tree will even make use of the functionality it provides.

Therefore I refactored the patch from bug 795288 to fallback to the old behaviour if pefile doesn't exist. If pefile functionality is required in mozmill, mozmill can just add pefile as a dependency and it'll automatically work.

I don't have a Windows machine available to test this on, but the change is pretty simple.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8368168 [details] [diff] [review]
0001-Bug-963176-mozinstall-should-gracefully-fallback-if-.patch

Review of attachment 8368168 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozinstall/mozinstall/mozinstall.py
@@ +160,2 @@
>  
> +        if os.access(src, os.X_OK):

What is better with os.access() compared to the .exe check? I think we should still keep that given that we don't want to run this code for bash or other executable scripts. Also installers are always EXE files.

@@ +170,5 @@
> +                            data.update(string.entries)
> +                return 'BuildID' not in data
> +            except ImportError:
> +                # pefile not available, just assume a proper binary was passed in
> +                return True

This is somewhat hidden and no-one will notice that feature. We should at least update the docstring to mention that, and also the docs at readthedocs.

::: mozinstall/tests/test.py
@@ +84,5 @@
> +                # test stub browser file
> +                # without pefile on the system this test will fail
> +                import pefile
> +                stub_exe = os.path.join(here, 'build_stub', 'firefox.exe')
> +                self.assertFalse(mozinstall.is_installer(stub_exe))

Given that we do not install pefile for travis checks, we will never get this tested. Please make sure to run another pre-build task to get it installed.

Comment 10

[Jarek Śmiejczak [:jotes]]

Comment on attachment 8368168 [details] [diff] [review]
0001-Bug-963176-mozinstall-should-gracefully-fallback-if-.patch

Review of attachment 8368168 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Andrew and Henrik... I'm sorry that my relatively small fix caused so much trouble.
However, i want to express my thoughts on this matter:
I don't think that leaving this as an optional feature is a way to go, because most of users propably won't notice missing package if we don't warn them. I think we/you should strongly consider removal of my patch and reopening old bug for this feature (on which i can still work).
After I figure out some better solution i'll pass it again to your review.
Again, i'm sorry and thanks for your hardwork.

::: mozinstall/mozinstall/mozinstall.py
@@ -17,5 @@
>  
>  import mozfile
>  import mozinfo
>  
> -import pefile

I would change this part a little into:
try:
  pefile
  has_pefile = True
except ImportError:
  has_pefile = False

@@ -178,2 @@
>      elif mozinfo.isWin:
> -        if src.lower().endswith('.exe'):

and then:
if src.lower().endswith('.exe') and has_pefile:
 (...)
elif if src.lower().endswith('.exe'):
  return True
return zipfile.is_zipfile(src)

It would be a little better than inlining of whole function. I'm thinking from readability perspective.

::: mozinstall/setup.py
@@ -15,5 @@
>  
>  deps = ['mozinfo >= 0.7',
>          'mozfile >= 1.0',
> -        'mozprocess >= 0.15',
> -        'pefile >= 1.2.10'

How user can know about pefile if  it's totally removed from setup.py. I think we should leave as an optional package in setup.py.

Comment 11

[Andrew Halberstadt [:ahal]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> What is better with os.access() compared to the .exe check? I think we
> should still keep that given that we don't want to run this code for bash or
> other executable scripts. Also installers are always EXE files.

I could create a text file called foo.exe. Checking the executable bit is the proper way to do this.

> This is somewhat hidden and no-one will notice that feature. We should at
> least update the docstring to mention that, and also the docs at readthedocs.

It isn't a new feature, it's how mozinstall works right now. It's the pefile part that is new and should be documented. I can upload a new patch that mentions pefile in the docstring though.

> Given that we do not install pefile for travis checks, we will never get
> this tested. Please make sure to run another pre-build task to get it
> installed.

The whole point of this patch is that we *can't* install pefile during testing. That is what will cause bustage in m-c. The travis job is going to be shutdown as soon as the m-c sync finishes, so we shouldn't worry about what happens there.

Comment 12

[Andrew Halberstadt [:ahal]]

(In reply to Jarek Śmiejczak from comment #10)
> Hi Andrew and Henrik... I'm sorry that my relatively small fix caused so
> much trouble.

It's ok, no worries.

> I don't think that leaving this as an optional feature is a way to go,
> because most of users propably won't notice missing package if we don't warn
> them.

Tbh, I don't think many people are going to be using this feature. As I understand it, this is mainly about the ability to install stub-installer type binaries, and I'm not sure what would need to do that other than mozmill. So if it were up to me, I'd just do something quick and dirty and call it a day. But if you want to back out your patch and try to fix it properly, I won't stop you :)

> How user can know about pefile if  it's totally removed from setup.py. I
> think we should leave as an optional package in setup.py.

Unfortunately I don't think there is a concept of "optional" packages in setuptools. Just having pefile in the list is what will cause our build jobs to fail.

Comment 13

[Andrew Halberstadt [:ahal]]

Attachment: 0001-Bug-963176-mozinstall-should-gracefully-fallback-if-.patch

I also changed the other public facing methods to be sphinx compliant.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8369454 [details] [diff] [review]
0001-Bug-963176-mozinstall-should-gracefully-fallback-if-.patch

Review of attachment 8369454 [details] [diff] [review]:
-----------------------------------------------------------------

Still not a big fan of hiding this feature given that we also release on pypi with a lot more consumers. I just checked readthedocs and we even don't have a section for mozinstall yet. So this will be totally hidden. So we might want to check how it works over the curse of the next weeks.

Personally I would have preferred to add the pefile code to mozinstall. It's not that large and would have remained the feature. But the bigger question here for the future will really be how strict do we want to be to get new features implemented which need external dependencies, e.g. for bug 911048 which is kinda blocker for us. I don't want to have to create a duplicated mozinstall package only for those purposes.

r=me with the change as pointed out below.

::: mozinstall/mozinstall/mozinstall.py
@@ +160,5 @@
>      elif mozinfo.isWin:
> +        if zipfile.is_zipfile(src):
> +            return True
> +
> +        if os.access(src, os.X_OK):

As mentioned last time we should not only do this check. So lets really check for an exe file too. Otherwise the following would also return True:

python -c "import os; print os.access('a.bat', os.X_OK)"

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrew Halberstadt [:ahal] from comment #12)
> Tbh, I don't think many people are going to be using this feature. As I
> understand it, this is mainly about the ability to install stub-installer
> type binaries, and I'm not sure what would need to do that other than

No, the stub installer is bug 963176. This feature here is really to detect if an EXE file is an installer or an already installed Firefox build.

> Unfortunately I don't think there is a concept of "optional" packages in
> setuptools. Just having pefile in the list is what will cause our build jobs
> to fail.

At least for developers we could include a requirements.txt file in the root folder of mozinstall, which contains optional packages to install. But that won't help end-users who install mozinstall via pypi.

Comment 16

[Andrew Halberstadt [:ahal]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> Personally I would have preferred to add the pefile code to mozinstall. It's
> not that large and would have remained the feature. But the bigger question
> here for the future will really be how strict do we want to be to get new
> features implemented which need external dependencies, e.g. for bug 911048
> which is kinda blocker for us. I don't want to have to create a duplicated
> mozinstall package only for those purposes.

If you'd prefer to solve it properly, that's fine with me. It's just that I want to move forward with the mozbase to m-c sync and this is one of the last things blocking that. So if you want to back out the original change temporarily and re-land it, that's fine by me. I don't have a preference either way as long as this patch lands or the original patch is backed out by sometime next week :)

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't have the time right now to do anything for this bug. So please go ahead and make sure to update the patch for my last review comments (check for exe file) and we can get it landed. We should follow-up soon with docs for mozinstall, so that those will be available on readthedocs.

Comment 18

[Andrew Halberstadt [:ahal]]

Thanks! I updated the patch to include .exe check and landed:
https://github.com/mozilla/mozbase/commit/900204af34ed2d2c0adab0dccf1411904f374cb5

Sorry for making this hidden.. I'll file a follow-up bug to solve this better.