Add psutil Python package to mozilla-central

RESOLVED FIXED in mozilla18

Status

defect
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla18
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 13 obsolete attachments)

607.69 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.47 KB, patch
Details | Diff | Splinter Review
2.10 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.36 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.48 KB, patch
Details | Diff | Splinter Review
4.03 KB, patch
ted
: review+
Details | Diff | Splinter Review
Assignee

Description

7 years ago
https://code.google.com/p/psutil/wiki/Documentation

I'm salivating over the possibilities. One-line memory usage? Yeah, that would have been nice when implementing linker memory measuring.

I think it would be cool to spin up a background thread during builds, test runs, etc that measured process resource utilization and report that as part of the build.

Anyway, this is an awesome tool. Let's put it to use.

Try at https://tbpl.mozilla.org/?tree=Try&rev=3d105a18481c

I suspect the Python developer headers (required for compiling) aren't on the buildbot machines. Will file releng bugs if we want to do this.
Assignee

Comment 1

7 years ago
Surprisingly, the Try build only failed on the ARM 7 builders \o/
Please make it optional, and enable it in the mozconfigs, it would be bad to start requiring python development files just to build firefox.
Assignee

Comment 3

7 years ago
(In reply to Mike Hommey [:glandium] from comment #2)
> Please make it optional, and enable it in the mozconfigs, it would be bad to
> start requiring python development files just to build firefox.

Yeah, I can do that. Although, I'd really like to get this running on buildbot. Let's just say I have my build splendid branch recording CPU, I/O, and memory metrics every second during builds. It can also correlate that to tiers and directories in the make traversal :D I just need a decent UI. Maybe I'll use the HTTP server built-in to Python's standard library along with some nifty JS graphing library...

I don't feel like writing configure magic to detect Python headers. The Python packaging tools have their own magic for this and I don't think it's worth duplicating. I think I'll just add a capability to our virtualenv populator that tries to install packages. If it fails, it continues instead of bailing. You OK with this approach?
Magic things are bad. Either you want the feature and it has to fail when it can't build, or you don't, and you certainly don't expect it to be enabled just because you happen to have the python headers installed. So I'm more in favor of a configure arg or just an AC_SUBST, depending how discoverable you want it to be. Then add the required ac_add_options or export to mozconfigs.

Comment 5

7 years ago
With this in mozilla-central, we should assess what can be done with mozbase/mozprocess with this module and possibly depend on it there
Assignee

Comment 6

7 years ago
(In reply to Jeff Hammel [:jhammel] from comment #5)
> With this in mozilla-central, we should assess what can be done with
> mozbase/mozprocess with this module and possibly depend on it there

psutil.POpen() wraps subprocess.POpen() and returns a psutil.Process, which has all the lovely APIs for querying state.
Assignee

Comment 7

7 years ago
Posted patch alpha autoconf magic (obsolete) — Splinter Review
I suck at autoconf. This is the best I could come up with. I know it isn't good enough to add to the tree. Do you think you could take it to the next level for me?

I also threw in the virtualenv changes so you can see where I'm going with this. This assumes psutil is at python/psutil. So, if you want to test, you'll need to  uncompress a psutil archive there or hack up packages.txt.
Attachment #657182 - Flags: feedback?(mh+mozilla)
Comment on attachment 657182 [details] [diff] [review]
alpha autoconf magic

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

That's not very much worse than anything else in the tree. But it will fail to do the right thing when cross-compiling.

::: configure.in
@@ +3089,5 @@
> +
> +for flag in $_PYTHON_LIBS; do
> +    case $flag in
> +    -lpython*)
> +        python_lib=`echo $flag | sed 's/^-l//'`

You're not using that variable. (and it lacks an underscore at the beginning)
Attachment #657182 - Flags: feedback?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #8)
> That's not very much worse than anything else in the tree. But it will fail
> to do the right thing when cross-compiling.

Note that there aren't autoconf macros to correctly handle the cross-compiling case. Modifying ac_compile is the closest to something that can work without too much hassle.
Something like:
_save_ac_compile=${ac_compile}
ac_compile='${HOST_CC-cc} -c $HOST_CFLAGS $HOST_CPPFLAGS conftest.$ac_ext 1>&5'

... do the test ...

ac_compile=${_save_ac_compile}
Assignee

Comment 11

7 years ago
This plays with fire. See discussion in bug 659881. I think this is better for everybody.

By using the version from the actual Python, we should ensure maximum compatibility. In other words, we default to something that should work instead of defaulting to something that might work. If systems (e.g. buildbot hosts) want to force a version, they can do so via a mozconfig file. We /may/ need to do this as part of this patch. I'll let a try build fail first.
Assignee: nobody → gps
Attachment #657182 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #658344 - Flags: review?(mh+mozilla)
Assignee

Comment 12

7 years ago
Pretty straightforward, I think.
Attachment #658345 - Flags: review?(mh+mozilla)
Assignee

Comment 13

7 years ago
I just added the archive from upstream unaltered.
Attachment #658346 - Flags: review?(mh+mozilla)
Assignee

Comment 14

7 years ago
One packages.txt line does the compilation. The other one sets up the pth file. Therefore, no egg-info directories.

Try at http://tbpl.mozilla.org/?tree=Try&rev=f1b128f26923
Attachment #658347 - Flags: review?(mh+mozilla)
Assignee

Updated

7 years ago
Depends on: 659881
Comment on attachment 658344 [details] [diff] [review]
Part 1: Change how MACOSX_DEPLOYMENT_TARGET is set, v1

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

I'm not very fond of the idea, because it enforces MACOSX_DEPLOYMENT_TARGET to what python was built against, which is completely irrelevant to what we are trying to build. Moreover, if you *do* need to build for an older osx version than what python was built for, you'd now be screwed, instead of being able to build what you want.
In essence, the problem is that we don't have a different MACOSX_DEPLOYMENT_TARGET for host and target builds. We do want a specific version for target builds, and don't really care for host builds. Host builds are where we'd want MACOSX_DEPLOYMENT_TARGET not to be set, and are what the virtualenv population are.
I think MACOSX_DEPLOYMENT_TARGET is better kept as it currently is.

::: configure.in
@@ +900,2 @@
>  MOZ_ARG_ENABLE_STRING(macos-target,
> +                      [  --enable-macos-target=VER (default=$osx_deployment_target_system)

These kind of things don't actually work. The help string of a MOZ_ARG_ENABLE_STRING is placed at the beginning of configure, while the variable assignment happens much later.
Attachment #658344 - Flags: review?(mh+mozilla) → review-
Comment on attachment 658345 [details] [diff] [review]
Part 2: Detect Python SDK in configure, v1

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

::: configure.in
@@ +3109,5 @@
> +
> +    _SAVE_CFLAGS="$CFLAGS"
> +    _SAVE_LDFLAGS="$LDFLAGS"
> +    CFLAGS="$CFLAGS $_PYTHON_CFLAGS"
> +    LDFLAGS="$LDFLAGS $_PYTHON_LDFLAGS"

Please use HOST_CFLAGS/HOST_LDFLAGS, and set ac_link to '${HOST_CC-cc} -o conftest${ac_exeext} $HOST_CFLAGS $HOST_CPPFLAGS $HOST_LDFLAGS conftest.$ac_ext $LIBS 1>&5'
Attachment #658345 - Flags: review?(mh+mozilla) → feedback+
Attachment #658346 - Flags: review?(mh+mozilla) → review+
Comment on attachment 658347 [details] [diff] [review]
Part 4: Add psutil to virtualenv, v1

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

::: .gitignore
@@ +49,5 @@
> +
> +# Python virtualenv artifacts.
> +python/psutil/.*.so
> +python/psutil/.*.o
> +python/psutil/.*.dll

What is virtualenv placing there exactly? Can we do force it not to do that, or to do it elsewhere? why no .obj?
Assignee

Comment 18

7 years ago
When you build C extensions, it will perform the original compilation in a build/ directory. On OS X, it produces the files:

python/psutil//build
python/psutil//build/lib.macosx-10.8-x86_64-2.7
python/psutil//build/lib.macosx-10.8-x86_64-2.7/_psutil_osx.so
python/psutil//build/lib.macosx-10.8-x86_64-2.7/_psutil_posix.so
python/psutil//build/temp.macosx-10.8-x86_64-2.7
python/psutil//build/temp.macosx-10.8-x86_64-2.7/psutil
python/psutil//build/temp.macosx-10.8-x86_64-2.7/psutil/_psutil_common.o
python/psutil//build/temp.macosx-10.8-x86_64-2.7/psutil/_psutil_osx.o
python/psutil//build/temp.macosx-10.8-x86_64-2.7/psutil/_psutil_posix.o
python/psutil//build/temp.macosx-10.8-x86_64-2.7/psutil/arch
python/psutil//build/temp.macosx-10.8-x86_64-2.7/psutil/arch/osx
python/psutil//build/temp.macosx-10.8-x86_64-2.7/psutil/arch/osx/process_info.o

You'll notice that the packages.txt line has --inplace defined. This causes the build to copy the .so into the same directory as setup.py, yielding:

python/psutil/_psutil_osx.so
python/psutil/_psutil_posix.so

This enables you to import things since python/psutil is added as part of a .pth.

Ideally these things would be installed to the virtualenv to not pollute srcdir. Unfortunately, I don't believe there is an "install just the C extensions" command for setuptools. I suppose we could hack something into our virtualenv population logic if we really wanted to. But, I think this might be somewhat hacky and fragile.
Assignee

Comment 19

7 years ago
OK. No more mucking around with the global MACOSX_DEPLOYMENT_TARGET. Instead, we now set this variable to the same as Python's only when populating the virtualenv.

Keep in mind this change only affects Python packages with C extensions. Currently we have none. After all parts of this bug land, we'll have psutil. And even that will be optional.

One would think that the old behavior of not setting MACOSX_DEPLOYMENT_TARGET would work. However, on my machine (which has Python 2.7.3 built from source and Python's MACOSX_DEPLOYMENT_TARGET set to 10.8), I get the following error when building psutil through configure:

Populating Python virtualenv
running build_ext
building '_psutil_osx' extension
/usr/local/llvm/bin/clang -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fcolor-diagnostics -DXP_UNIX -DXP_MACOSX -DNO_X11 -I/usr/local/bin/../Cellar/python/2.7.3/include/python2.7 -c psutil/_psutil_osx.c -o build/temp.macosx-10.8-x86_64-2.7/psutil/_psutil_osx.o
Traceback (most recent call last):
  File "/Users/gps/src/services-central/python/psutil/setup.py", line 159, in <module>
    main()
  File "/Users/gps/src/services-central/python/psutil/setup.py", line 156, in main
    setup(**setup_args)
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/core.py", line 152, in setup
    dist.run_commands()
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/dist.py", line 953, in run_commands
    self.run_command(cmd)
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "/Users/gps/src/services-central/obj-ff-dbg/_virtualenv/lib/python2.7/site-packages/setuptools-0.6c11-py2.7.egg/setuptools/command/build_ext.py", line 46, in run
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/command/build_ext.py", line 339, in run
    self.build_extensions()
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/command/build_ext.py", line 448, in build_extensions
    self.build_extension(ext)
  File "/Users/gps/src/services-central/obj-ff-dbg/_virtualenv/lib/python2.7/site-packages/setuptools-0.6c11-py2.7.egg/setuptools/command/build_ext.py", line 175, in build_extension
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/command/build_ext.py", line 498, in build_extension
    depends=ext.depends)
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/ccompiler.py", line 572, in compile
    self._compile(obj, src, ext, cc_args, extra_postargs, pp_opts)
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/unixccompiler.py", line 178, in _compile
    extra_postargs)
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/ccompiler.py", line 873, in spawn
    spawn(cmd, dry_run=self.dry_run)
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/spawn.py", line 34, in spawn
    _spawn_posix(cmd, search_path, dry_run=dry_run)
  File "/usr/local/Cellar/python/2.7.3/lib/python2.7/distutils/spawn.py", line 122, in _spawn_posix
    if _cfg_target_split > [int(x) for x in cur_target.split('.')]:
ValueError: invalid literal for int() with base 10: ''
Traceback (most recent call last):
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 85, in <module>
    populate_virtualenv(sys.argv[1], sys.argv[2], have_c)
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 61, in populate_virtualenv
    handle_package(package)
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 58, in handle_package
    handle_package(package[1:])
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 49, in handle_package
    package[2:])
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 77, in call_setup
    raise Exception('Error installing package: %s' % directory)
Exception: Error installing package: /Users/gps/src/services-central/python/psutil
*** Fix above errors and then restart with               "make -f client.mk build"

I can't not set MACOSX_DEPLOYMENT_TARGET as part of the command line because I get:

Populating Python virtualenv
running build_ext
building '_psutil_osx' extension
creating build
creating build/temp.macosx-10.8-x86_64-2.7
creating build/temp.macosx-10.8-x86_64-2.7/psutil
creating build/temp.macosx-10.8-x86_64-2.7/psutil/arch
creating build/temp.macosx-10.8-x86_64-2.7/psutil/arch/osx
/usr/local/llvm/bin/clang -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fcolor-diagnostics -DXP_UNIX -DXP_MACOSX -DNO_X11 -I/usr/local/bin/../Cellar/python/2.7.3/include/python2.7 -c psutil/_psutil_osx.c -o build/temp.macosx-10.8-x86_64-2.7/psutil/_psutil_osx.o
error: $MACOSX_DEPLOYMENT_TARGET mismatch: now "10.6" but "10.8" during configure
Traceback (most recent call last):
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 85, in <module>
    populate_virtualenv(sys.argv[1], sys.argv[2], have_c)
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 61, in populate_virtualenv
    handle_package(package)
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 58, in handle_package
    handle_package(package[1:])
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 49, in handle_package
    package[2:])
  File "/Users/gps/src/services-central/build/virtualenv/populate_virtualenv.py", line 77, in call_setup
    raise Exception('Error installing package: %s' % directory)
Exception: Error installing package: /Users/gps/src/services-central/python/psutil
*** Fix above errors and then restart with               "make -f client.mk build"

I suppose we could hold off exporting MACOSX_DEPLOYMENT_TARGET. But, that seems like more pain than it's worth.
Attachment #658344 - Attachment is obsolete: true
Attachment #658523 - Flags: review?(mh+mozilla)
Assignee

Comment 20

7 years ago
Incorporated feedback.

Parts 3 and 4 are identical to previous (I still need to investigate .obj on Windows and will add a new patch for part 4 if I need to add version control ignores).

Try at https://tbpl.mozilla.org/?tree=Try&rev=7c78aeeb75f9
Attachment #658345 - Attachment is obsolete: true
Attachment #658524 - Flags: review?(mh+mozilla)
Assignee

Comment 21

7 years ago
As part of the latest try run, I discovered that sysconfig is only available in Python 2.7. I'm going to change part 1 to be dependent on Python 2.7. Since the build system should soon require 2.7, I think this will be acceptable.

Anyway, I'm sure there is a more efficient way to do this. But, I'm not sure if autoconf has a built-in split or string parsing without invoking extra shells.
Attachment #658561 - Flags: review?(mh+mozilla)
Assignee

Comment 22

7 years ago
Comment on attachment 658523 [details] [diff] [review]
Part 1: Set MACOSX_DEPLOYMENT_TARGET when building virtualenv, v2

sysconfig is only present in Python 2.7. Will need some additional work here.
Attachment #658523 - Flags: review?(mh+mozilla)
Assignee

Comment 23

7 years ago
Now conditional on Python 2.7.

I suppose there is a way to get at this information with Python 2.6 and below. But, presumably builds will soon require 2.7, so I'm not too worried about supporting it.
Attachment #658523 - Attachment is obsolete: true
Attachment #658569 - Flags: review?(mh+mozilla)
Assignee

Comment 24

7 years ago
Now passing whether to build c extensions as an argument not an environment variable.

Note that C extension support is still failing on try due to compile failures in AC_TRY_LINK. This may not be the final version of this patch.
Attachment #658524 - Attachment is obsolete: true
Attachment #658524 - Flags: review?(mh+mozilla)
Attachment #658570 - Flags: review?(mh+mozilla)
Assignee

Comment 25

7 years ago
Python 2.5 doesn't have namedtuple. So, use index access.
Attachment #658561 - Attachment is obsolete: true
Attachment #658561 - Flags: review?(mh+mozilla)
Attachment #658590 - Flags: review?(mh+mozilla)
Assignee

Comment 26

7 years ago
I should probably test these things before submitting a patch for review. Silly square brackets and autoconf.
Attachment #658590 - Attachment is obsolete: true
Attachment #658590 - Flags: review?(mh+mozilla)
Attachment #658594 - Flags: review?(mh+mozilla)
Assignee

Comment 27

7 years ago
Comment on attachment 658594 [details] [diff] [review]
Part 0: Capture Python version in configure, v3

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

::: configure.in
@@ +1796,5 @@
>  changequote(,)
> +python_version_major=`$PYTHON -c 'import sys; print sys.version_info[0]'`
> +python_version_minor=`$PYTHON -c 'import sys; print sys.version_info[1]'`
> +python_version_micro=`$PYTHON -c 'import sys; print sys.version_info[2]'`
> +-changequote([,])

Ignore the leading -. It just isn't my day.
Assignee

Comment 28

7 years ago
It turns out that Python on the OS X builders is broken (bug 788705). And, there doesn't appear to be a python-config.exe on Windows. So, I just reimplemented python-config minimally in a standalone .py file. I initially tried to inline it in configure.in, but the escaping, etc got too ugly for my tastes.

With all these patches applied, this is how things fare:

* OS X - works locally and on build infra \o/
* Linux - works locally. Build infra fails to detect Python SDK because of linking issue. Not sure if the .so is missing or what. The Linux builders are running Python 2.5, so I expect them to break.
* Windows - Does not work locally nor on the build infra. It fails to detect the Python SDK.

As part of investigating this, I pushed a Try build that tries to build psutil by ignoring the result of Python SDK detection: https://tbpl.mozilla.org/?tree=Try&rev=0bf33ffa65f6

Ignore the reds on that build because I put an "exit 1" at the bottom of configure to save try resources. And, I also added extra debugging which throws off the log parsers. If you dig into the logs, you'll see that psutil managed to build for Linux, Android Armv6, Android opt, Android debug, and all the Windows builders! However, Armv7a ICS opt and debug both failed because Python.h couldn't be found. I'll have to file a bug for that. Basically, this means that the "Python SDK" detection in this patch is wrong.

Unfortunately, I have no clue how to fix it short of trying to build the extension and ignoring (rather than aborting) if it fails. There is just so much magic happening inside distutils and I think trying to duplicate that inside configure is just asking for trouble.

Anyway, the try for this patch series modulo the configure abort is at https://tbpl.mozilla.org/?tree=Try&rev=ecfe65f305da. As I stated earlier, only OS X should work.
Attachment #658570 - Attachment is obsolete: true
Attachment #658570 - Flags: review?(mh+mozilla)
Attachment #658711 - Flags: review?(mh+mozilla)
Assignee

Comment 29

7 years ago
I pushed a Try that changes the behavior to try to build extensions and silently ignores failures. It /should/ work on all the builders except Armv7a (because they don't have the Python development files). http://tbpl.mozilla.org/?tree=Try&rev=da2d5f60dad1

I also filed bug 788908 to track installing the Python development files on the Armv7a machines.

Arguably, we may want a single "action" in packages.txt to denote build_ext + .pth so they are done as an atomic unit. In the case of psutil, |import psutil| will fail with an ImportError because the main psutil module tries to import the binary extension. This fails if this is missing. So, while psutil's .py files are in the virtualenv, we ultimately fail.
Assignee

Comment 30

7 years ago
Adding people from the Python brain trust who may be able to help here. Comments 19 and 28 have the context.

Basically, what's the best way to test whether we can compile Python C extensions without going through setup.py for an actual package?
Comment on attachment 658569 [details] [diff] [review]
Part 1: Set MACOSX_DEPLOYMENT_TARGET when building virtualenv, v3

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

I don't know how likely it is that future versions of virtualenv would end up breaking with an empty MACOSX_DEPLOYMENT_TARGET, but we might as well use the same thing for it.
Attachment #658569 - Flags: review?(mh+mozilla) → review+
Comment on attachment 658594 [details] [diff] [review]
Part 0: Capture Python version in configure, v3

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

::: configure.in
@@ +1796,4 @@
>  changequote(,)
> +python_version_major=`$PYTHON -c 'import sys; print sys.version_info[0]'`
> +python_version_minor=`$PYTHON -c 'import sys; print sys.version_info[1]'`
> +python_version_micro=`$PYTHON -c 'import sys; print sys.version_info[2]'`

Something like the following should work, and only run python once:
read python_version_major python_version_minor python_version_micro <<EOF
`$PYTHON -c 'import sys; print sys.version_info[0],sys.version_info[1],sys.version_info[2]'`
EOF

@@ +1799,5 @@
> +python_version_micro=`$PYTHON -c 'import sys; print sys.version_info[2]'`
> +-changequote([,])
> +
> +if test $python_version_major -ne $PYTHON_VERSION_MAJOR; then
> +    AC_MSG_ERROR([Cannot build on Python $python_version_major.])

Please use the same error message as below.
Attachment #658594 - Flags: review?(mh+mozilla) → review+
Comment on attachment 658711 [details] [diff] [review]
Part 2: Detect Python SDK in configure, v4

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

More eyes on python_extensions_flags.py can't hurt.
Attachment #658711 - Flags: review?(ted.mielczarek)
Attachment #658711 - Flags: review?(mh+mozilla)
Attachment #658711 - Flags: review+
(In reply to Gregory Szorc [:gps] from comment #30)
> Adding people from the Python brain trust who may be able to help here.
> Comments 19 and 28 have the context.
> 
> Basically, what's the best way to test whether we can compile Python C
> extensions without going through setup.py for an actual package?

I am not sure to understand this question, but running through setup.py is mandatory because that's how the distutils machinery initializes its compiler to create the extension -- plus people do things sometimes in setup.py that affect compilation.

So I would say: you can't test if you can compile a Python C extension until you try 

The simplest way to try it is to do:

$ python setup.py build_ext -i 

and see if you get your .so files in the tree alongside the .c files
Assignee

Comment 35

7 years ago
(In reply to Tarek Ziadé (:tarek) from comment #34)
> I am not sure to understand this question, but running through setup.py is
> mandatory because that's how the distutils machinery initializes its
> compiler to create the extension -- plus people do things sometimes in
> setup.py that affect compilation.

I want to test whether we can compile *any* Python C detection. This typically boils down to trying to compile the most minimal Python C extension possible (just a call to Py_Initialize or similar). Unfortunately, I'm getting hung up because I don't know how to extract the right compiler flags to use. |python-config --cflags --ldflags| is giving me something that doesn't actually work. Or, in the case of Windows, AFAICT there is no python-config, so I'm not sure how exactly to obtain the compiler/linker arguments to use.

I do know that on these same machines, building an extension (psutil) through setup.py does work. And, that setup.py is simply populating different variables depending on the platform - nothing too crazy.

So, I guess my question is whether I can easily have Python try to compile a C extensions without setting up a dummy directory tree containing extension files. Is there a "compile this C file with the default compiler settings" API somewhere in the bowels of distutils?
Status: ASSIGNED → NEW
Assignee

Updated

7 years ago
Status: NEW → ASSIGNED
Assignee

Comment 36

7 years ago
This is an alternate version of part 2. It adds "optional" and "optional-skipnext" actions to the virtualenv manifest.

Since we are having difficulty detecting generically whether we can build C extensions in configure, I figure we should just try to build C extensions and not abort if they fail.

Since this actually works to build psutil on the buildbot hosts, I prefer this version.

I would still like generic "can we build Python C extensions" detection in configure. But, until there is a robust way to do that, I think this is our best option.

Try at https://tbpl.mozilla.org/?tree=Try&rev=9846f5f53b1c
Attachment #658952 - Flags: review?(mh+mozilla)
Assignee

Comment 37

7 years ago
Python's behavior on variable assignment and closures gets me all the time. Kinda wish Python had explicit variable declaration...

New try at https://tbpl.mozilla.org/?tree=Try&rev=0829d13ecaa1
Attachment #658952 - Attachment is obsolete: true
Attachment #658952 - Flags: review?(mh+mozilla)
Attachment #658953 - Flags: review?(mh+mozilla)
Assignee

Comment 38

7 years ago
This is part of my preferred "b track" for this bug. It uses optional-skipnext to install psutil.

Since the last patch, I also updated the VCS ignore files to catch things properly, including on Windows.
Attachment #658956 - Flags: review?(mh+mozilla)
Assignee

Comment 39

7 years ago
More Python scoping BS. Apparently you need Python 3 to declare a non-global variable as non-local. The workaround is to use a mutable type. Head asplode.

Try at http://tbpl.mozilla.org/?tree=Try&rev=fdac65e18a97
Attachment #658953 - Attachment is obsolete: true
Attachment #658953 - Flags: review?(mh+mozilla)
Attachment #658961 - Flags: review?(mh+mozilla)
Assignee

Comment 40

7 years ago
I found a bug in psutil's reporting of I/O wait on OS X: https://code.google.com/p/psutil/issues/detail?id=323. I'll see about upstreaming the fix and adding the fix to this bug.
Comment on attachment 658961 [details] [diff] [review]
Part 2b: Add optional virtualenv population actions, v3

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

::: build/virtualenv/populate_virtualenv.py
@@ +28,5 @@
> +          "optional" field is stripped then the remaining line is processed
> +          like normal. e.g. "optional:setup.py:python/foo:built_ext:-i"
> +
> +      optional-skipnext -- This is like optional except the action immediately
> +         following this one in the manifest is skipped if this action fails.

I think i'd be more comfortable if the marker was on the next action. That is, make the action optional depending on the result of the previous. Ted, what do you think?
Attachment #658961 - Flags: review?(ted.mielczarek)
Attachment #658961 - Flags: review?(mh+mozilla)
Attachment #658961 - Flags: feedback-
Comment on attachment 658956 [details] [diff] [review]
Part 4b: Add psutil to virtualenv, v1

Resetting, pending discussion on part 2b.
Attachment #658956 - Flags: review?(mh+mozilla)
Comment on attachment 658347 [details] [diff] [review]
Part 4: Add psutil to virtualenv, v1

This is replaced by part 4b, right?
Attachment #658347 - Flags: review?(mh+mozilla)

Comment 44

7 years ago
> +      optional-skipnext -- This is like optional except the action immediately
> +         following this one in the manifest is skipped if this action fails.

I can't say I'm a fan.  I generally feel when domain specific languages are making one-offs like this semantic, its time to rethink a bit
Assignee

Comment 45

7 years ago
(In reply to Jeff Hammel [:jhammel] from comment #44)
> > +      optional-skipnext -- This is like optional except the action immediately
> > +         following this one in the manifest is skipped if this action fails.
> 
> I can't say I'm a fan.  I generally feel when domain specific languages are
> making one-offs like this semantic, its time to rethink a bit

I don't like it either.

FWIW, I invented packages.txt because it was less evil than a Makefile and was super simple. It has since grown features beyond what I originally envisioned. In the context of the new world, I think it should be rewritten to use Preprocessor.py or similar. But, I think that is the domain of a follow-up bug.

(In reply to Mike Hommey [:glandium] from comment #43)
> Comment on attachment 658347 [details] [diff] [review]
> Part 4: Add psutil to virtualenv, v1
> 
> This is replaced by part 4b, right?

Yes.
Comment on attachment 658961 [details] [diff] [review]
Part 2b: Add optional virtualenv population actions, v3

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

::: build/virtualenv/populate_virtualenv.py
@@ +28,5 @@
> +          "optional" field is stripped then the remaining line is processed
> +          like normal. e.g. "optional:setup.py:python/foo:built_ext:-i"
> +
> +      optional-skipnext -- This is like optional except the action immediately
> +         following this one in the manifest is skipped if this action fails.

Yeah, I don't like this either. I'm fine with "optional", but "optional-skipnext" is a bit too much for me.
Attachment #658961 - Flags: review?(ted.mielczarek) → review-
Assignee

Comment 47

7 years ago
No skipnext.

I propose the compromise be to make both the build_ext and .pth actions for psutil optional. We can file a follow-up bug to convert packages.txt to use Preprocess.py where I imagine we'll solve this by having sections composed of multiple actions. If an action in a section fails, that section is aborted.

Until that is ready, psutil will have a .pth created if build_ext fails. However, |import psutil| will raise ImportError because it won't be able to load the compiled extension file since it is missing. If we do it properly, it will still raise an ImportError but it will be because there is no psutil package in site-packages because the .pth is missing. The end result is the same, so I think this is an acceptable compromise.
Attachment #658711 - Attachment is obsolete: true
Attachment #658961 - Attachment is obsolete: true
Attachment #658711 - Flags: review?(ted.mielczarek)
Assignee

Comment 48

7 years ago
That last patch was completely busted due to my stupidity.
Attachment #661348 - Attachment is obsolete: true
Attachment #661353 - Flags: review?(ted.mielczarek)
Attachment #661353 - Flags: review?(ted.mielczarek) → review+

Updated

7 years ago
Blocks: 803834
Assignee

Updated

6 years ago
Blocks: 870575

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.