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

Some more OpenCV fixes #56

Merged
merged 6 commits into from
Jan 21, 2020
Merged

Some more OpenCV fixes #56

merged 6 commits into from
Jan 21, 2020

Conversation

rajat2004
Copy link
Collaborator

@rajat2004 rajat2004 commented Jan 1, 2020

... Part 3
Issue #52

  1. Add Travis build for OpenCV with Anaconda, also test FFmpeg & OpenCV installation
  2. Rename conflicting shared libraries of Anaconda, after building, restore the old names
  3. Add entry for FFmpeg libs in ld.so.conf.d
  4. Use curly braces for grouping commands, runs code in the current shell
  5. General installation improvements
  6. Shift nasm, yasm install to apt, remove su $USER

@rajat2004
Copy link
Collaborator Author

rajat2004 commented Jan 1, 2020

Do we really need the Xenial builds? I don't feel we're getting any info from them which is not there in the Bionic ones. Maybe remove some of them? Just a thought
Edit: Well, it is useful when comparing library versions to check if the right ones are being used

@rajat2004
Copy link
Collaborator Author

Okay, this is getting frustrating now, download of bat in the first script often fails, maybe due to redirection or something, it's okay when it was just the 1st build, but now need to rerun a 1hr 40min build.
This is the current error message-

Installing bat, a handy replacement for cat
================================
Exception caught
Exception: [download_helper.cc:451] errorCode=1 Unrecognized URI or unsupported protocol: https://github.com{"event_type":"authentication.click","payload":{"location_in_page":"site

The command "CIINSTALL=yes ./1-BasicSetUp.sh" exited with 1.

@rajat2004 rajat2004 added the WIP label Jan 2, 2020
@rajat2004 rajat2004 requested a review from rsnk96 January 2, 2020 04:43
@rajat2004
Copy link
Collaborator Author

rajat2004 commented Jan 2, 2020

make is having a problem with harfbuzz, even though CMake seems to be loading the correct system library (1.7.2 vs Anaconda's 1.8.8) -

/opt/anaconda3/lib/libharfbuzz.so.0: undefined reference to `FT_Done_MM_Var'
collect2: error: ld returned 1 exit status
apps/visualisation/CMakeFiles/opencv_visualisation.dir/build.make:92: recipe for target 'bin/opencv_visualisation' failed
make[2]: *** [bin/opencv_visualisation] Error 1
CMakeFiles/Makefile2:6472: recipe for target 'apps/visualisation/CMakeFiles/opencv_visualisation.dir/all' failed
make[1]: *** [apps/visualisation/CMakeFiles/opencv_visualisation.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

Problem with tbb on Xenial

/opt/anaconda3/lib/libtbb.so.2: undefined reference to `__cxa_init_primary_exception@CXXABI_1.3.11'
/opt/anaconda3/lib/libtbb.so.2: undefined reference to `std::__exception_ptr::exception_ptr::exception_ptr(void*)@CXXABI_1.3.11'
collect2: error: ld returned 1 exit status
apps/annotation/CMakeFiles/opencv_annotation.dir/build.make:92: recipe for target 'bin/opencv_annotation' failed
make[2]: *** [bin/opencv_annotation] Error 1
CMakeFiles/Makefile2:6356: recipe for target 'apps/annotation/CMakeFiles/opencv_annotation.dir/all' failed

Note that the job may show it has passed but the problem could still be there, since there's no set -e option set in the OpenCV script

@rajat2004
Copy link
Collaborator Author

rajat2004 commented Jan 2, 2020

Okay, this is getting frustrating now, download of bat in the first script often fails, maybe due to redirection or something, it's okay when it was just the 1st build, but now need to rerun a 1hr 40min build.
This is the current error message-

Installing bat, a handy replacement for cat
================================
Exception caught
Exception: [download_helper.cc:451] errorCode=1 Unrecognized URI or unsupported protocol: https://github.com{"event_type":"authentication.click","payload":{"location_in_page":"site

The command "CIINSTALL=yes ./1-BasicSetUp.sh" exited with 1.

Did a bit of testing for this, shell script used is given below -

tries=20
for i in $(seq $tries); do 
	latest_bat_setup=$(wget -q -O - https://github.com/sharkdp/bat/releases/ index.html | grep "deb" | head -n 1 | cut -d \" -f 2)
	# latest_bat_setup=$(curl --silent "https://api.github.com/repos/sharkdp/bat/releases/latest" | grep "deb" | grep "browser_download_url" | head -n 1 | cut -d \" -f 4)
	echo $latest_bat_setup
done

Output with wget-

$ ./test.sh
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
btn btn-sm btn-with-count tooltipped tooltipped-s
tooltipped tooltipped-s btn btn-sm btn-with-count
btn btn-sm btn-with-count tooltipped tooltipped-s
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
{"event_type":"authentication.click","payload":{"location_in_page":"site header","repository_id":null,"auth_type":"SIGN_UP","client_id":null,"originating_request_id":"C6D4:1DC7:263567F:37A2952:5E0DCE35","originating_url":"https://github.com/sharkdp/bat/releases","referrer":null,"user_id":null}}
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
btn btn-sm btn-with-count tooltipped tooltipped-s
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
btn btn-sm btn-with-count tooltipped tooltipped-s
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb
/sharkdp/bat/releases/download/v0.12.1/bat-musl_0.12.1_amd64.deb

curl gave correct output every time, and was a bit faster (possibly because it wasn't downloading the the entire html file), therefore committed the change

Side note: bat is really great!! Haven't explored the themes and options much, but still great! Already aliased
Edit: Well, removed the alias, but still

@rajat2004 rajat2004 force-pushed the test-opencv branch 2 times, most recently from 3341e79 to dbfc96f Compare January 2, 2020 13:20
@rajat2004 rajat2004 removed the WIP label Jan 2, 2020
@rajat2004
Copy link
Collaborator Author

rajat2004 commented Jan 3, 2020

I think that OpenCV is using the compiled FFmpeg libraries since that was the problem of #52, but have also mentioned in the first point in #20, with an example of zlib
We could make it and some of the others which are left (cmake, pkg-config) as shared libs with a bit of work, but I don't think that's a good thing to do since they are libs used throughout the system, and replacing them could cause more significant compatibility problems. Right now, only the codecs are replaced, and that shouldn't cause problems

Edit: The latest commit shouldn't make any difference in the build if it's all working as intended
Downloaded the log from Travis for the 2 builds, and compared with-

diff -y --suppress-common-lines without-libs.txt with-libs.txt

with-libs.txt
without-libs.txt
No diff other than the time, the building percentages, some useless difference, and of course the missing install lines

@rajat2004 rajat2004 force-pushed the test-opencv branch 2 times, most recently from 86226e7 to 15466a6 Compare January 3, 2020 13:25
@rsnk96
Copy link
Owner

rsnk96 commented Jan 3, 2020

Do we really need the Xenial builds? I don't feel we're getting any info from them which is not there in the Bionic ones. Maybe remove some of them? Just a thought
Edit: Well, it is useful when comparing library versions to check if the right ones are being used

Yes, would recommend keeping 16.04 at least till 20.04 reaches decent adoption

@rsnk96
Copy link
Owner

rsnk96 commented Jan 3, 2020

Side note: bat is really great!! Haven't explored the themes and options much, but still great! Already aliased
Edit: Well, removed the alias, but still

Would actually recommend keeping the alias :)

@rajat2004
Copy link
Collaborator Author

Do we really need the Xenial builds? I don't feel we're getting any info from them which is not there in the Bionic ones. Maybe remove some of them? Just a thought
Edit: Well, it is useful when comparing library versions to check if the right ones are being used

Yes, would recommend keeping 16.04 at least till 20.04 reaches decent adoption

Yeah, was just a thought I had after looking at 10 builds 😅 . Even then, meant "all the Xenial builds", should have mentioned that, cancelling specific builds works just as well. Can't say against this, since I was also using 16.04 till about a month ago

Would actually recommend keeping the alias :)

Do have a bit of habit of using cat for copying files, aliasing will remove that habit at least :). Already added back the alias

@rsnk96
Copy link
Owner

rsnk96 commented Jan 3, 2020

Do have a bit of habit of using cat for copying files, aliasing will remove that habit at least :). Already added back the alias

cat followed by xclip?

@rajat2004
Copy link
Collaborator Author

rajat2004 commented Jan 3, 2020

Do have a bit of habit of using cat for copying files, aliasing will remove that habit at least :). Already added back the alias

cat followed by xclip?

Yeah, cat + aliased xclip -selection clipboard, and using redirection instead of mv for making a copy of file
Also, bat -p is a good alias for cat, doesn't add the filename at the top, highlighting is still there
Turns out bat automatically changes the behaviour to plain on piping, so just bat should be okay. Will need to start using mv for copying files

@rajat2004
Copy link
Collaborator Author

I think that OpenCV is using the compiled FFmpeg libraries since that was the problem of #52, but have also mentioned in the first point in #20, with an example of zlib
We could make it and some of the others which are left (cmake, pkg-config) as shared libs with a bit of work, but I don't think that's a good thing to do since they are libs used throughout the system, and replacing them could cause more significant compatibility problems. Right now, only the codecs are replaced, and that shouldn't cause problems

Edit: The latest commit shouldn't make any difference in the build if it's all working as intended
Downloaded the log from Travis for the 2 builds, and compared with-

diff -y --suppress-common-lines without-libs.txt with-libs.txt

with-libs.txt
without-libs.txt
No diff other than the time, the building percentages, some useless difference, and of course the missing install lines

Problem doesn't seem to be solved, the difference in library versions can be seen from the output of ffmpeg -version and OpenCV's build info

libavutil      56. 35.100 / 56. 35.100
libavcodec     58. 59.102 / 58. 59.102
libavformat    58. 33.100 / 58. 33.100
libavdevice    58.  9.100 / 58.  9.100
 Video I/O:
    DC1394:                      YES (2.2.4)
    FFMPEG:                      YES
      avcodec:                   YES (56.60.100)
      avformat:                  YES (56.40.101)
      avutil:                    YES (54.31.100)
      swscale:                   YES (3.1.101)

Did a test build with verbose output - https://travis-ci.com/rajat2004/Ubuntu-Setup-Scripts/builds/143082037
Installation of vtk installs the codecs as well

@rajat2004
Copy link
Collaborator Author

Did some more testing, with the vtk installation commented out, the codecs are not found by CMake, which uses pkg-config for this I think. The problem might be due to the paths not being updated in Travis since su - $USER is not being run inside it. But I'm also not sure that alone is the culprit.

While testing with Docker, there were problems with users and it creating new login session and stopping the execution of the script in between. Will need to work on making the Docker environment more resembling a system install. Or a different virtual env system.

Though that does bring up another point, CMake is finding the system libs whereas the linker errors if it doesn't locate the source built libs.
The last point about the PR itself is about the renaming of the Anaconda libs since quite a lot of them are renamed right now, so that might cause problems.
@rsnk96 Your thoughts on this?

@rajat2004
Copy link
Collaborator Author

The last commit can be dropped since it's somewhat hiding the issue that different codecs are being used, and committed later when confirmed that things are working as expected

@rajat2004
Copy link
Collaborator Author

Shit, such a simple thing which I overlooked. The Travis build may show different versions, but didn't even check on my system. This will teach me not to go into complicated things without first verifying the simple way.

OpenCV build info -

Video I/O:
    DC1394:                      YES (ver 2.2.5)
    FFMPEG:                      YES
      avcodec:                   YES (ver 58.59.102)
      avformat:                  YES (ver 58.33.100)
      avutil:                    YES (ver 56.35.100)
      swscale:                   YES (ver 5.6.100)

FFmpeg -

libavutil      56. 35.100 / 56. 35.100
libavcodec     58. 59.102 / 58. 59.102
libavformat    58. 33.100 / 58. 33.100
libavdevice    58.  9.100 / 58.  9.100
libavfilter     7. 62.100 /  7. 62.100
libswscale      5.  6.100 /  5.  6.100
libswresample   3.  6.100 /  3.  6.100
libpostproc    55.  6.100 / 55.  6.100

Changes in the PR are the same as the ones I did on my system, so I guess it should be okay
The Anaconda renaming is mostly the main concern then

@rsnk96
Copy link
Owner

rsnk96 commented Jan 8, 2020

The last point about the PR itself is about the renaming of the Anaconda libs since quite a lot of them are renamed right now, so that might cause problems.
@rsnk96 Your thoughts on this?

Renaming the anaconda libraries back to the original names after the compilation should work ideally, because during the build, it should link to the non-anaconda versions of the libraries, and that should be permanent (ideally, not sure how they are linking). Any chance you can test this?

@rsnk96
Copy link
Owner

rsnk96 commented Jan 8, 2020

The Travis build may show different versions, but didn't even check on my system. This will teach me not to go into complicated things without first verifying the simple way.

Haha looks like it might be something specific to how Travis handles things then

@rajat2004
Copy link
Collaborator Author

Renaming the anaconda libraries back to the original names after the compilation should work ideally, because during the build, it should link to the non-anaconda versions of the libraries, and that should be permanent (ideally, not sure how they are linking). Any chance you can test this?

Will try, I'm installing VirtualBox on my system today for proper testing, and to avoid some of the headaches I'm having right now with Docker. Will be useful for the Zsh+Zim update also
Not sure about the linking, but it is called dynamic linker, so might be the other way too. In the dynamic case, then mostly it would depend on which library it finds first

Haha looks like it might be something specific to how Travis handles things then

I'm having suspicions about the whole thing since the culprit seemed to be the paths, I added the LD_LIBRARY and other paths in the settings as was earlier (so maybe should have only removed the modification to PATH). But no difference, still the same
Anyways, the VirtualBox testing should be proof whether things are working or not

@rajat2004
Copy link
Collaborator Author

Wasn't able to get any testing done till now, VirtualBox installation and setup also took longer than expected
I'm going to split the commits like the bat and Travis update into separate branches, to check differences and to reduce to scope of this PR

@rajat2004
Copy link
Collaborator Author

Cleaned up the commits, but as mentioned, this doesn't change the paths, and so OpenCV finds the system libraries
I think it'll be cleaner to move the FFmpeg install to a different file, which can be called from here. An option or a cmd line arg could be added to ask whether to build FFmpeg from source or use system libraries
Plus the script by itself can be used just for FFmpeg installs

@rajat2004
Copy link
Collaborator Author

Another option is that we can execute the scripts using source, that'll modify the paths in the parent shell

@rajat2004
Copy link
Collaborator Author

rajat2004 commented Jan 19, 2020

@rsnk96 I think I've got it! Made 2 changes, first didn't execute the script but used source
Second was changing the braces from (...) to {...}
The last 2 commits - master...rajat2004:test-source
Travis build - https://travis-ci.com/rajat2004/Ubuntu-Setup-Scripts/builds/145110743
Edit: Well, there's a problem in this with the install location, which I think should b fixed after the latest commit

Source - https://askubuntu.com/questions/606378/when-to-use-vs-in-bash
I've cherry-picked the commits to this branch

@rajat2004
Copy link
Collaborator Author

It's working!

 FFMPEG:                      YES
      avcodec:                   YES (58.59.102)
      avformat:                  YES (58.33.100)
      avutil:                    YES (56.35.100)
      swscale:                   YES (5.6.100)
libavutil      56. 35.100 / 56. 35.100
libavcodec     58. 59.102 / 58. 59.102
libavformat    58. 33.100 / 58. 33.100
libavdevice    58.  9.100 / 58.  9.100
libavfilter     7. 62.100 /  7. 62.100
libswscale      5.  6.100 /  5.  6.100

Would like to have a final review done, then will clean up commits and add note in Readme for this

@rajat2004 rajat2004 force-pushed the test-opencv branch 3 times, most recently from c3f394b to d55e440 Compare January 20, 2020 06:53
@rajat2004
Copy link
Collaborator Author

Commits cleaned up a bit, note added

@rsnk96
Copy link
Owner

rsnk96 commented Jan 20, 2020

Awesome stuff! :)

@rajat2004
Copy link
Collaborator Author

@rsnk96 I think this PR is good to go, would be great to have your review once to see if anything is still missing or needs fixing

@rsnk96
Copy link
Owner

rsnk96 commented Jan 20, 2020

Hey. I updated my review questions inline (can be seen in Files Changed tab while using Github on a desktop). Here are the comments that you can pen your opinions on:

  1. Is the source-ing needed after the (...) --> {...} change too? If not, we need not recommend users run the opencv script using source? (in the readme)
  2. would recommend mv -- "$f" "${f/.so/.so.bkp}" keeping in accordance with general conventions
  3. will undoing the rename at the end of the program make opencv unusable? I feel if possible, we should do that

@rajat2004
Copy link
Collaborator Author

@rsnk96 Review comments are not visible :) I think you'll need to submit it again, that has occurred with me before as well

1. Is the source-ing needed after the `(...)` --> `{...}` change too? If not, we need not recommend users run the opencv script using source? (in the readme)

I think so, since source allows the script to change the environment of the current session. However, I haven't checked that since the order of commits was the opposite. Will test that!

2. would recommend mv -- "$f" "${f/.so/.so.bkp}" keeping in accordance with general conventions

Sure, will make the changes

3. will undoing the rename at the end of the program make opencv unusable? I feel if possible, we should do that

I'm not sure about this, will test!

@rajat2004
Copy link
Collaborator Author

  1. Is the source-ing needed after the (...) --> {...} change too? If not, we need not recommend users run the opencv script using source? (in the readme)

I think so, since source allows the script to change the environment of the current session. However, I haven't checked that since the order of commits was the opposite. Will test that!

It does seem to be working - https://travis-ci.com/rajat2004/Ubuntu-Setup-Scripts/builds/145216554

@rajat2004
Copy link
Collaborator Author

  1. will undoing the rename at the end of the program make opencv unusable? I feel if possible, we should do that

I'm not sure about this, will test!

Build running right now - https://travis-ci.com/rajat2004/Ubuntu-Setup-Scripts/builds/145223052

Also test FFmpeg and OpenCV installation after script completes
@rajat2004
Copy link
Collaborator Author

I've removed the source part, some of my assumptions or understanding of why it works was wrong, no need to source the script
Added the commit to undo the renaming, it does work in Travis, but not sure about it since ld also caches the libraries, so could be that it's masking the effect. Will need to test this properly once

@rsnk96
Copy link
Owner

rsnk96 commented Jan 21, 2020

Build running right now - https://travis-ci.com/rajat2004/Ubuntu-Setup-Scripts/builds/145223052

Looks like it worked! This is a good PR, feel free to merge it whenever you're ready

@rajat2004
Copy link
Collaborator Author

rajat2004 commented Jan 21, 2020

@rsnk96 I think it's all working correctly, tested in a docker container, OpenCV working correctly after undoing the rename, reboot, as well as ldconfig to clear cache
Also checked the linked libraries of one of the OpenCV .so files using ldd, it's linking to the system ones (see libtbb)
Screenshot from 2020-01-21 13-40-52

I'll mostly wait till tonight, for either of us to check something else if anything pops up, but looks okay to me as well

Edit: Squashed the Conda renaming commits

@rajat2004
Copy link
Collaborator Author

Merging, all builds look okay, more changes needed can be done in another PR 😅
Side note: The last Travis build of this PR was the 200th one

@rajat2004 rajat2004 merged commit f029da2 into rsnk96:master Jan 21, 2020
@rajat2004 rajat2004 deleted the test-opencv branch January 22, 2020 11:30
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.

2 participants