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

clean up python binding compilation #1795

Merged
merged 12 commits into from Dec 11, 2018

Conversation

Projects
None yet
4 participants
@trws
Copy link
Member

trws commented Nov 2, 2018

So far this generates a single compiled module for all of the core bindings, while leaving the security bindings separate since they are distributed separately. It should help with object compatibility between modules, and reduce how much one has to think about which ffi or lib object is being used.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 3, 2018

Codecov Report

Merging #1795 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1795      +/-   ##
==========================================
+ Coverage   79.89%   79.91%   +0.01%     
==========================================
  Files         196      196              
  Lines       35228    35228              
==========================================
+ Hits        28147    28151       +4     
+ Misses       7081     7077       -4
Impacted Files Coverage Δ
src/common/libjsc/jstatctl.c 87.72% <ø> (ø) ⬆️
src/common/libflux/message.c 81.64% <0%> (+0.12%) ⬆️
src/common/libflux/mrpc.c 87.89% <0%> (+1.17%) ⬆️
@SteVwonder
Copy link
Member

SteVwonder left a comment

Couple of comments, but LGTM otherwise. This is a great improvement! Much nicer to have 100% compatibility amongst the various modules (and to only have a single .so).

Sorry for the delay in reviewing.

for k in dir(lib):
if PATTERN.match(k):
v = ffi.string(getattr(lib, k)).decode("ascii")
print("adding", k, v)

This comment has been minimized.

@SteVwonder

SteVwonder Nov 29, 2018

Member

Looks like a debug print snuck in.

>>> from flux import jsc
('adding', 'JSC_JOBID', u'jobid')
('adding', 'JSC_RDESC', u'rdesc')
('adding', 'JSC_RDESC_NCORES', u'ncores')
('adding', 'JSC_RDESC_NGPUS', u'ngpus')
('adding', 'JSC_RDESC_NNODES', u'nnodes')
('adding', 'JSC_RDESC_NTASKS', u'ntasks')
('adding', 'JSC_RDESC_WALLTIME', u'walltime')
('adding', 'JSC_RDL', u'rdl')
('adding', 'JSC_R_LITE', u'R_lite')
('adding', 'JSC_STATE_PAIR', u'state-pair')
('adding', 'JSC_STATE_PAIR_NSTATE', u'nstate')
('adding', 'JSC_STATE_PAIR_OSTATE', u'ostate')
@@ -6,6 +6,7 @@ AM_CPPFLAGS = \
$(CODE_COVERAGE_CFLAGS)

AM_LDFLAGS = \
-L$(libdir) \

This comment has been minimized.

@SteVwonder

SteVwonder Nov 29, 2018

Member

Probably worth mentioning #1755 either in the specific commit that added this or in the PR's OP.

@@ -21,8 +21,12 @@

absolute_head = os.path.abspath(os.path.join(args.path, args.header))

def process_header(f):
def process_header(f, including_path='.'):
print (f, including_path)

This comment has been minimized.

@SteVwonder

SteVwonder Nov 29, 2018

Member

Looks like another debug print that snuck in.

Show resolved Hide resolved src/bindings/python/make_binding.py
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 6, 2018

Where do we stand on this PR? I'm fine with this going in once @SteVwonder is happy wtih it.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 10, 2018

Sorry for the delay, I'm testing the build fixes again after the merge, as soon as that comes back green I'll update here.

@trws trws force-pushed the trws:add-python-command branch from 49f8c13 to 9bccd1f Dec 10, 2018

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Dec 10, 2018

Ok, this should be ready for a re-review @SteVwonder.

@SteVwonder SteVwonder merged commit f38482b into flux-framework:master Dec 11, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 52068c6...296992a
Details
codecov/project 79.91% (+0.01%) compared to 52068c6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

SteVwonder added a commit to SteVwonder/flux-core that referenced this pull request Dec 21, 2018

python/makefile.am: remove unused and unrecognized variables
The `AM_LIBADD` variable is unrecognized by autotools.  It can be safely
removed since `PYTHON_LDFLAGS` are already included in the `common_libs`
variable, which is in-turn included in the `LIBADD` of all libraries'.

The _job variables and targets were vestiges from the reduction to a
single shared library in flux-framework#1795.  They are unused and can be safely
removed as well.

Closes flux-framework#1869

SteVwonder added a commit to SteVwonder/flux-core that referenced this pull request Dec 21, 2018

python/makefile.am: remove unused and unrecognized variables
The `AM_LIBADD` variable is unrecognized by autotools.  It can be safely
removed since `PYTHON_LDFLAGS` are already included in the `common_libs`
variable, which is in-turn included in the `LIBADD` of all libraries'.

The _job variables and targets were vestiges from the reduction to a
single shared library in flux-framework#1795.  They are unused and can be safely
removed as well.

Closes flux-framework#1869

garlick added a commit to SteVwonder/flux-core that referenced this pull request Dec 21, 2018

python/makefile.am: remove unused and unrecognized variables
The `AM_LIBADD` variable is unrecognized by autotools.  It can be safely
removed since `PYTHON_LDFLAGS` are already included in the `common_libs`
variable, which is in-turn included in the `LIBADD` of all libraries'.

The _job variables and targets were vestiges from the reduction to a
single shared library in flux-framework#1795.  They are unused and can be safely
removed as well.

Closes flux-framework#1869
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.