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

cleanup: fix in-tree includes for idset.h, subprocess.h and other small fixes #2014

Merged
merged 8 commits into from Feb 15, 2019

Conversation

@grondo
Copy link
Contributor

commented Feb 14, 2019

Just a few collected cleanups:

  • remove some leftovers from libjobspec
  • ensure in-tree code can include <flux/idset.h>
  • ensure subprocess.h is included from <flux/core.h> since it is in libflux-core.so (end result: do not include subprocess.h directly anymore)

This is still a WIP because I'm not sure if my quick fix for the python bindings is even close to correct.

@grondo grondo changed the title wip: tidying up some include files, stale files and directories wip: fixup in-tree includes for idset.h, subprocess.h and other small fixes Feb 14, 2019
@grondo grondo force-pushed the grondo:konmari-puchi branch from b60ee8c to 9b0222a Feb 14, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

I added another questionable python binding change. See ef5c669.

Also updated .mergify.yml to remove codecov/project as a condition, since we've seen codecov.io delay its status updates (and fixed the test for pr titles with "wip" I think)

@grondo grondo requested review from trws and SteVwonder Feb 14, 2019
@grondo grondo force-pushed the grondo:konmari-puchi branch from 9b0222a to cbe60b7 Feb 14, 2019
Copy link
Member

left a comment

LGTM.

My review adds little value on the python stuff, though the fact that tests are passing seems like a good indication that those changes are probably OK.

@grondo grondo force-pushed the grondo:konmari-puchi branch from cbe60b7 to b21373b Feb 14, 2019
@SteVwonder

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

grondo wants to merge 7 commits into flux-framework:master from grondo:konmari-puchi

konmari-puchi?

I'm not sure if my quick fix for the python bindings is even close to correct

So the short story is: I think what you did, @grondo, is the right thing. The logner story: the make_binding.py starts at core.h and follows all #includes that use double quotes (as opposed to brackets) to build a "mega_header" that includes the contents of every flux header reachable from core.h. Since #include <sys/types.h> is ignored, it doesn't get included in the generated _core.c. Seeing as including the contents of every system header would just be excessive to solve this problem, I think manually defining pid_t is totally fine.

Related: I noticed that subprocess.h has pid_t as a return type, but the header itself does not include <sys/types.h>, just the subprocess.c file does. Is that correct? Naively, it seems to me that the subprocess header should be the one to include <sys/types.h>. (Note: this doesn't solve the python problem, I'm just wondering since I stumbled across it).

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

konmari-puchi?

Sorry, a play on that "Tidying Up" show -- KonMari is the method of tidying up and I looked up "puchi" --means something like "a little bit" in Japanese.

Related: I noticed that subprocess.h has pid_t as a return type, but the header itself does not include <sys/types.h>

You are correct this should be added to subprocess.h. I actually had a fix for this in my branch, but dropped it along the way when trying out fixes for python bindings.

@grondo grondo force-pushed the grondo:konmari-puchi branch 2 times, most recently from 8467ea8 to 0085cf4 Feb 15, 2019
@SteVwonder

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

a play on that "Tidying Up" show -- KonMari is the method of tidying up and I looked up "puchi" --means something like "a little bit" in Japanese.

That's clever. I like that. 😄

@grondo grondo force-pushed the grondo:konmari-puchi branch from 0085cf4 to 5482991 Feb 15, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

Whoa something went very awry with my last push, trying again:

checking Major version... 0085cf4
checking Minor version... 4
checking Point version... 

I wonder if we hit some corner case with git describe output and the AX_SPLIT_VERSION macro?

grondo added 8 commits Feb 13, 2019
Remove a couple of leftovers from when libjobspec was pulled
out of flux-core:

 - stale .gitignore in src/common/libjobspec kept the directory around
 - the jobspec.hpp header wrapper was still in /usr/src/include

Fixes #2009
Problem: Source can't include idset.h header the same way they would
out-of-tree, i.e. <flux/idset>

Add the idset.h "wrapper" in src/include/flux/idset.h to match what
is done for optparse.h.

Fixes #2007
Problem: libsubprocess is compiled into libflux-core.so, but -- unlike
core/kvs.h and core/job.h -- the header file subprocess.h has to be
separately included into any source using that interface.

Add subprocess.h to src/common/core.h *and* src/include/flux/core.h
so that flux subprocess interfaces are automatically included with
flux/core.h both in-tree and for installed flux-core.
Hint to python bindings that pid_t is an integer type.
libsubprocess is part of libflux-core.so, and thus, similar to
kvs.h and job.h, its header subprocess.h should not be included
directly when <flux/core.h> is already included.

Remove redundant #includes of this header from throughout the
flux-core codebase.
Codecov.io sometimes fails or takes a long time to report status
back after a rebase. To avoid lengthy delays in merge of PRs, remove
the codecov/project condition from mergify.yml. Reviewers will just
have to ensure that coverage is acceptable before issuing a positive
review or adding the merge-when-passing label.

Also, fix the regex for matching PR titles that start with
"wip", "WIP", "[wip", etc.
The references to cffi_modules _jsc_build.py:ffi and
_kvs_build.py:ffi seem to be stale. Remove them.
Problem: flux/subprocess.h uses pid_t without also including system
headers which define pid_t type.

Pull in <sys/types.h> which should be sufficient to get the pid_t
definition.
@grondo grondo force-pushed the grondo:konmari-puchi branch from 5482991 to 8746c04 Feb 15, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Feb 15, 2019

Codecov Report

Merging #2014 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2014      +/-   ##
==========================================
- Coverage   80.58%   80.53%   -0.05%     
==========================================
  Files         180      180              
  Lines       28914    28914              
==========================================
- Hits        23299    23286      -13     
- Misses       5615     5628      +13
Impacted Files Coverage Δ
src/cmd/builtin/proxy.c 74.55% <ø> (ø) ⬆️
src/broker/exec.c 65% <ø> (ø) ⬆️
src/broker/runlevel.c 81% <ø> (ø) ⬆️
src/cmd/flux-start.c 78.21% <ø> (ø) ⬆️
src/modules/job-ingest/worker.c 65.03% <ø> (ø) ⬆️
src/modules/cron/task.c 79.31% <ø> (ø) ⬆️
src/cmd/flux-exec.c 75.78% <ø> (ø) ⬆️
src/common/libflux/ev_flux.c 97.56% <0%> (-2.44%) ⬇️
src/common/libflux/msg_handler.c 89.06% <0%> (-1.18%) ⬇️
src/connectors/local/local.c 88.57% <0%> (-0.72%) ⬇️
... and 6 more
@grondo grondo changed the title wip: fixup in-tree includes for idset.h, subprocess.h and other small fixes cleanup: fix in-tree includes for idset.h, subprocess.h and other small fixes Feb 15, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

I see you removed the WIP so in it goes! Thanks!

@garlick garlick merged commit bbe342a into flux-framework:master Feb 15, 2019
5 checks passed
5 checks passed
Mergify — disabled due to configuration change Mergify configuration has been modified
Details
Mergify — future config checker The new Mergify configuration is valid
Details
codecov/patch Coverage not affected when comparing e40e896...8746c04
Details
codecov/project 80.53% (-0.05%) compared to e40e896
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grondo grondo deleted the grondo:konmari-puchi branch Feb 16, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

👊

@trws

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@garlick garlick referenced this pull request Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.