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

src/python/bcc/version.py created as part of build #1964

Closed
bolinfest opened this Issue Sep 10, 2018 · 13 comments

Comments

Projects
None yet
6 participants
@bolinfest
Contributor

bolinfest commented Sep 10, 2018

I'm on Ubuntu 18.04.1. First, note that the build instructions:

# Trusty and older
VER=trusty
echo "deb http://llvm.org/apt/$VER/ llvm-toolchain-$VER-3.7 main
deb-src http://llvm.org/apt/$VER/ llvm-toolchain-$VER-3.7 main" | \
  sudo tee /etc/apt/sources.list.d/llvm.list
wget -O - http://llvm.org/apt/llvm-snapshot.gpg.key | sudo apt-key add -
sudo apt-get update

# All versions
sudo apt-get -y install bison build-essential cmake flex git libedit-dev \
  libllvm3.7 llvm-3.7-dev libclang-3.7-dev python zlib1g-dev libelf-dev

# For Lua support
sudo apt-get -y install luajit luajit-5.1-dev

mention Trusty (14.04) and older, so I wasn't sure what to do for 18.04. I ended up copypasta'ing some stuff from the LLVM Debian/Ubuntu nightly packages and created /etc/apt/sources.list.d/llvm.list with the following contents:

# i386 not available
deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic main
deb-src http://apt.llvm.org/bionic/ llvm-toolchain-bionic main
# 6.0
deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-6.0 main
deb-src http://apt.llvm.org/bionic/ llvm-toolchain-bionic-6.0 main
# 7
deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-7 main
deb-src http://apt.llvm.org/bionic/ llvm-toolchain-bionic-7 main

Then I basically followed the existing instructions, but instead of:

sudo apt-get -y install libllvm3.7 llvm-3.7-dev libclang-3.7-dev

I did:

sudo apt-get -y install libllvm7 llvm7-dev libclang-7-dev

(Note that I originally tried this with 8 instead of 7, but then building bcc failed because the getLocStart() API appears to have been renamed in LLVM 8.)

Now the problem was getting things to build because everything seems hardcoded for LLVM 3.7. I rooted around in CMakeLists.txt and found $LLVM_LIBRARY_DIRS, so adding an extra argument to cmake worked fine:

cmake .. -DCMAKE_INSTALL_PREFIX=/usr -DLLVM_LIBRARY_DIRS=/usr/lib/llvm-7/lib

However, when I was done, git status was not clean, as there was the file src/python/bcc/version.py with the following contents:

__version__ = '0.7.0-8cc0bda6'

Not a huge deal, but it would be nice if this file were either not created, created under build/, or listed in .gitignore.

It would also be nice if the Ubuntu build instructions were updated to call out what to do for versions after Trusty. Hopefully the record of the steps I went through (and the upcoming breaking in LLVM 8) are useful here.

@yonghong-song

This comment has been minimized.

Show comment
Hide comment
@yonghong-song

yonghong-song Sep 10, 2018

Collaborator

Yes, this can be easily reproduced. Somehow, when I change branch and did a rebuild. This src/python/bcc/version.py will show up. I do not know why either.

Collaborator

yonghong-song commented Sep 10, 2018

Yes, this can be easily reproduced. Somehow, when I change branch and did a rebuild. This src/python/bcc/version.py will show up. I do not know why either.

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Sep 10, 2018

Contributor

I noticed that as well, but didn't took the time to debug it. I think it's been happening since #1826 got merged.

Contributor

pchaigno commented Sep 10, 2018

I noticed that as well, but didn't took the time to debug it. I think it's been happening since #1826 got merged.

@yonghong-song

This comment has been minimized.

Show comment
Hide comment
@yonghong-song

yonghong-song Sep 10, 2018

Collaborator

@pchaigno Yes, #1826 introduced a version.py in the source tree to generate a version.py at build the directory.

Collaborator

yonghong-song commented Sep 10, 2018

@pchaigno Yes, #1826 introduced a version.py in the source tree to generate a version.py at build the directory.

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Sep 10, 2018

Contributor

Looks like adding that file to .gitignore would be the right fix here since it's generated at build time from version.py.in...?

Contributor

pchaigno commented Sep 10, 2018

Looks like adding that file to .gitignore would be the right fix here since it's generated at build time from version.py.in...?

@bolinfest

This comment has been minimized.

Show comment
Hide comment
@bolinfest

bolinfest Sep 10, 2018

Contributor

@pchaigno Adding it to .gitignore would certainly be the quickest fix, though it always seems a bit dirty to me to have generated code in the src/ folder as opposed to the build/ folder, but maybe it's not worth the effort to move it?

Contributor

bolinfest commented Sep 10, 2018

@pchaigno Adding it to .gitignore would certainly be the quickest fix, though it always seems a bit dirty to me to have generated code in the src/ folder as opposed to the build/ folder, but maybe it's not worth the effort to move it?

@pchaigno

This comment has been minimized.

Show comment
Hide comment
@pchaigno

pchaigno Sep 10, 2018

Contributor

@bolinfest Right, it probably would be best to generate that file in build/. @andihit Was this the intended behavior? Would it be possible to generate version.py in the build directory only?

Contributor

pchaigno commented Sep 10, 2018

@bolinfest Right, it probably would be best to generate that file in build/. @andihit Was this the intended behavior? Would it be possible to generate version.py in the build directory only?

@yonghong-song

This comment has been minimized.

Show comment
Hide comment
@yonghong-song

yonghong-song Sep 10, 2018

Collaborator

We could add an entry in .gitignore. But looking at the source code, I don't understand why this can happen? Any idea?

Collaborator

yonghong-song commented Sep 10, 2018

We could add an entry in .gitignore. But looking at the source code, I don't understand why this can happen? Any idea?

myllynen added a commit to performancecopilot/pcp that referenced this issue Sep 12, 2018

pmdabcc: harden BCC version check
In some cases the version number might not be what is expected so
try to provide proper BCC version strings only.

iovisor/bcc#1964
@brendangregg

This comment has been minimized.

Show comment
Hide comment
@brendangregg

brendangregg Sep 12, 2018

Member

this issue is annoying my builds as well...

Member

brendangregg commented Sep 12, 2018

this issue is annoying my builds as well...

@andihit

This comment has been minimized.

Show comment
Hide comment
@andihit

andihit Sep 12, 2018

Contributor

@pchaigno Basically I followed the same approach as it's used for creating setup.py from setup.py.in (https://github.com/iovisor/bcc/blob/master/src/python/CMakeLists.txt#L14)

Yes, build/ directory would be fine also.
Any idea why the setup.py is cleaned after the build but bcc/version.py is not?

I'll see if I get time in a few days, probably we just need another directory insead of ${CMAKE_CURRENT_BINARY_DIR} in the cmake file.

Contributor

andihit commented Sep 12, 2018

@pchaigno Basically I followed the same approach as it's used for creating setup.py from setup.py.in (https://github.com/iovisor/bcc/blob/master/src/python/CMakeLists.txt#L14)

Yes, build/ directory would be fine also.
Any idea why the setup.py is cleaned after the build but bcc/version.py is not?

I'll see if I get time in a few days, probably we just need another directory insead of ${CMAKE_CURRENT_BINARY_DIR} in the cmake file.

@mauriciovasquezbernal

This comment has been minimized.

Show comment
Hide comment
@mauriciovasquezbernal

mauriciovasquezbernal Sep 25, 2018

Contributor

@andihit you are actually generating version.py in the build/ folder, but there is a link between the build/src/python/bcc and the src/python/bcc folders -> https://github.com/iovisor/bcc/blob/master/src/python/CMakeLists.txt#L8.

I think just adding version.py to .gitignore will solve those issues.

Contributor

mauriciovasquezbernal commented Sep 25, 2018

@andihit you are actually generating version.py in the build/ folder, but there is a link between the build/src/python/bcc and the src/python/bcc folders -> https://github.com/iovisor/bcc/blob/master/src/python/CMakeLists.txt#L8.

I think just adding version.py to .gitignore will solve those issues.

@yonghong-song

This comment has been minimized.

Show comment
Hide comment
@yonghong-song

yonghong-song Sep 26, 2018

Collaborator

Sounds good then, could you submit a pull request for this? Could you double check when we do a make clean, we do clean this file as well? Thanks!

Collaborator

yonghong-song commented Sep 26, 2018

Sounds good then, could you submit a pull request for this? Could you double check when we do a make clean, we do clean this file as well? Thanks!

@mauriciovasquezbernal

This comment has been minimized.

Show comment
Hide comment
@mauriciovasquezbernal

mauriciovasquezbernal Oct 4, 2018

Contributor

I think this can be close after #1989.

Contributor

mauriciovasquezbernal commented Oct 4, 2018

I think this can be close after #1989.

@yonghong-song

This comment has been minimized.

Show comment
Hide comment
@yonghong-song

yonghong-song Oct 5, 2018

Collaborator

Just closed the issue!

Collaborator

yonghong-song commented Oct 5, 2018

Just closed the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment