Skip to content
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

Python json and constant rework #799

Merged
merged 4 commits into from Sep 22, 2016

Conversation

Projects
None yet
5 participants
@trws
Copy link
Member

trws commented Sep 8, 2016

This PR reworks constants so they are no longer doubly defined, while maintaining the same interface, removes json_c bindings, and cleans up a lurking out-of-tree build testing bug that has apparently been lurking for a while.

@garlick garlick added the review label Sep 8, 2016

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 8, 2016

Thanks! Compiles on hype with cffi-1.1.x, but dies in make check:

Traceback (most recent call last):
  File "../src/bindings/python/test_commands/test_runner.t", line 53, in <module>
    run_under_dir(result_set, '../test', 'test')
  File "../src/bindings/python/test_commands/test_runner.t", line 29, in run_under_dir
    (mod_prefix, m.group(1))))
  File "/opt/python-2.7/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/g/g0/grondo/git/f/src/bindings/python/test/wrapper.py", line 6, in <module>
    import flux.json_c
ImportError: No module named json_c

Also, perhaps commit message for 3b6f91b needs an update? It says "rework constant handling" but it just seems to add some import calls. (I'm not a python guy so apologies if I missed something obvious!)

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 8, 2016

Thanks @grondo, not sure how check succeeded for me, maybe I still had a
dirty path or something. Will post a fix and a rebase shortly.

On 8 Sep 2016, at 13:04, Mark Grondona wrote:

Thanks! Compiles on hype with cffi-1.1.x, but dies in make check:

Traceback (most recent call last):
  File "../src/bindings/python/test_commands/test_runner.t", line 53, 
in <module>
    run_under_dir(result_set, '../test', 'test')
  File "../src/bindings/python/test_commands/test_runner.t", line 29, 
in run_under_dir
    (mod_prefix, m.group(1))))
  File "/opt/python-2.7/lib/python2.7/importlib/__init__.py", line 37, 
in import_module
    __import__(name)
  File "/g/g0/grondo/git/f/src/bindings/python/test/wrapper.py", line 
6, in <module>
    import flux.json_c
ImportError: No module named json_c

Also, perhaps commit message for 3b6f91b needs an update? It says
"rework constant handling" but it just seems to add some import
calls. (I'm not a python guy so apologies if I missed something
obvious!)

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#799 (comment)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 8, 2016

Coverage Status

Coverage increased (+0.008%) to 74.978% when pulling 37a6cdf on trws:python-constant-rework into 3fd9da0 on flux-framework:master.

@trws trws force-pushed the trws:python-constant-rework branch from 37a6cdf to 64be838 Sep 8, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 8, 2016

Current coverage is 74.93% (diff: 100%)

Merging #799 into master will decrease coverage by 0.02%

@@             master       #799   diff @@
==========================================
  Files           145        145          
  Lines         24911      24911          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          18674      18667     -7   
- Misses         6237       6244     +7   
  Partials          0          0          

Powered by Codecov. Last update 914fb9f...0ff5d70

@trws trws force-pushed the trws:python-constant-rework branch from 64be838 to 09a8993 Sep 8, 2016

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 8, 2016

One thing to note, a few constants that used to be macros are now static constants in this PR. This has not caused any problems for me, and wont unless they're used in a non-executable context, but it's technically a change to the exposed headers. If this is a problem, I can special case those macros and have the binding generator create function wrappers for them, but this seemed more forward-portable. Another alternative would be to convert them into enums if that would be preferable, which might bring back the ability to reference them in a static initializer.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 8, 2016

Coverage Status

Coverage remained the same at 74.97% when pulling 09a8993 on trws:python-constant-rework into 3fd9da0 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 12, 2016

@trws if we could have the incremental changes to C headers squashed into one (standalone) commit instead of hopping from #define to const to enum, I'd appreciate it.

Same with the out of tree install bug fix. Is this #805 (just opened - saw this last night and was going to follow up). Please reference that if so.

Getting rid of the json-c stuff and the doubly defined constants is good cleanup!

@trws trws force-pushed the trws:python-constant-rework branch 2 times, most recently from 5aaf626 to e93126b Sep 14, 2016

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 14, 2016

I think this addresses the cleanup you mention @garlick. The build update may help with #805, but that looks more like a permission problem than an issue with files being missed in transfer.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 14, 2016

Evidently there's an issue with the travis version of cffi I wasn't seeing in my environment where calculated enum values are concerned, this version should take care of it.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.04%) to 75.275% when pulling 9f5a1eb on trws:python-constant-rework into 430bedb on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 14, 2016

Travis is still failing, although the PR is looking nicely factored now.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 14, 2016

Travis is now dying on #805. Looking into it, but it doesn't reproduce directly for me locally, so there may be a couple of rounds with travis working this out.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.06%) to 75.299% when pulling f76b1ec on trws:python-constant-rework into 430bedb on flux-framework:master.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 14, 2016

Turns out my earlier solution works fine with out of tree check, but not with distcheck due to inheriting the read-only properties from the dist source directory. Should be good to go at this point.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 14, 2016

Great! Want to squash the two message.c changes, and the two build system changes?

trws added some commits Sep 14, 2016

python: excise unnecessary json-c bindings
Thanks to updates in the kvs interface the json-c bindings are no longer
necessary to support the python bindings, rather python is now using
completely native json handling.  Big thanks to @SteVwonder for getting
the ball rolling on this.
python: automatic detection and exposure of consts
All enums and constants matching "FLUX_.*" are now exposed rather than
replicated.

@trws trws force-pushed the trws:python-constant-rework branch from f76b1ec to 0ff5d70 Sep 22, 2016

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 22, 2016

Somehow this fell off my radar for a bit. Rebased and should be good to go.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 22, 2016

Coverage Status

Coverage decreased (-0.03%) to 75.253% when pulling 0ff5d70 on trws:python-constant-rework into 914fb9f on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 22, 2016

This one looks good to me. I did have a problem where I checked out your branch but did not run make clean and the python bindings didn't have the new constants, but I don't think that is a new problem, and shouldn't hold up this PR.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 22, 2016

Actually, one comment: I'm not sure how strongly I feel about this, but I guess the new enum wrappers for constants will be in a public API so perhaps they should have flux_ prepended or something?, e.g. enum flux_nodeid not just enum nodeid?

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 22, 2016

I don't see why not, I can add that real quick.

By the way, the other PR, #807, should address the issue with the bindings not getting updated during a build/check.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 22, 2016

By the way, the other PR, #807, should address the issue with the bindings not getting updated during a build/check.

Oh, awesome!

trws added some commits Sep 14, 2016

messages/python: constant macros -> enums
Macros are not well handled by the python binding generator, enums on
the other hand work quite well, macros converted to enums to support
automatic constant detection and exposure through the bindings.  Note
that older versions of cffi apparently can't handle constant
calculations, including bitwise negation, and c99 doesn't support binary
literals, so the nodeids are now hex constants.
python: out of tree build fix
Some core files for the python bindings were not being placed correctly
for out of tree builds, this should also fix the permission errors in
travis.

@trws trws force-pushed the trws:python-constant-rework branch from 0ff5d70 to 63ce4b0 Sep 22, 2016

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Sep 22, 2016

When I went to update the enum names, I noticed the others are all anonymous enums, so I made these follow that convention.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 22, 2016

When I went to update the enum names, I noticed the others are all anonymous enums, so I made these follow that convention.

That works for me, sorry I should have suggested that

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 22, 2016

Thanks for this work @trws - I'll merge as soon as travis turns green.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 22, 2016

Coverage Status

Coverage decreased (-12.03%) to 63.25% when pulling 63ce4b0 on trws:python-constant-rework into 914fb9f on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 22, 2016

The clang build failed here, which is an odd place that in any case would seem to have nothing to do with this PR. The other builds were OK so I just restarted it.

not ok 71 - stat watcher invoked once for size chnage
#   Failed test 'stat watcher invoked once for size chnage'
#   at ../../../../src/common/libflux/test/reactor.c line 644.
ok 72 - stat watcher invoked once for nlink set to zero
ok 73 - destroying reactor then watcher doesn't segfault
1..73
# Looks like you failed 1 test of 73 run.
FAIL: test_reactor.t
===================
1 of 7 tests failed
===================

@garlick garlick merged commit 170d0b1 into flux-framework:master Sep 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 75.257%
Details

@garlick garlick removed the review label Sep 22, 2016

@garlick garlick referenced this pull request Oct 26, 2016

Closed

0.5.0 release notes #879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.