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

cmake/python: fix build race condition #2111

Merged
merged 2 commits into from
Jan 10, 2019

Conversation

SaveTheRbtz
Copy link
Contributor

@SaveTheRbtz SaveTheRbtz commented Jan 4, 2019

A followup for the build flakes from #2102 (comment)

Rewritten CMakeLists.txt for the python code. Now it:

  • autodiscovers .py and .py.in files, so there is no need to hardcode them there.
  • copies everything to a separate directory for each python provided.
  • does not have PREVIOUS_PY logic.
  • runs in parallel.

Also while here removed the MANIFEST file since:

  • it is outdated.
  • distutils regenerates it anyway.

Tested locally with:

cmake ../src/python -DPYTHON_CMD="python;python2;python3;python2.7;python3.5"
while :; do make clean; make -j || break; done; echo $?
^C
make clean; make -j; make install

Please note that I have no idea what i'm doing, since this is the first time I'm touching CMake, not to mention using it to build python extensions. It would be nice if someone who actually knows cmake could take a look at this diff.
Main known issue is why do we build sdist in the first place, since I do not see it being used anywhere and maybe we should use build instead?

cc: @yonghong-song
ref: #1951 #2102

@yonghong-song
Copy link
Collaborator

[buildbot, test this please]

@SaveTheRbtz
Copy link
Contributor Author

[buildbot, test this please]

Did you consider enabling build automatically? If there is a concern about the Jenkins abuse you can consider switching it from a whitelist to blacklist.

@yonghong-song
Copy link
Collaborator

make sense. Let us discuss in the iovisor bi-weekly meeting to find an appropriate approach.

@yonghong-song
Copy link
Collaborator

@drzaeus77 could you take a look?

@yonghong-song
Copy link
Collaborator

I checked and tested the patch for full build with multiple python commands. It works fine. The newly changed makefile has separate build and install directory for each python command. The old way has one build directory and multiple install directories. The new way is cleaner and safer as well.

@yonghong-song yonghong-song merged commit 9b3b127 into iovisor:master Jan 10, 2019
brendangregg pushed a commit to brendangregg/bcc that referenced this pull request Jan 11, 2019
* python: remove MANIFEST

* cmake/python: fix build race condition
palexster pushed a commit to palexster/bcc that referenced this pull request Jul 7, 2019
* python: remove MANIFEST

* cmake/python: fix build race condition
CrackerCat pushed a commit to CrackerCat/bcc that referenced this pull request Jul 31, 2024
* python: remove MANIFEST

* cmake/python: fix build race condition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants