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

Add snapcraft.yaml file #776

Closed
wants to merge 5 commits into from
Closed

Add snapcraft.yaml file #776

wants to merge 5 commits into from

Conversation

mardy
Copy link
Contributor

@mardy mardy commented Feb 9, 2017

This is the recipe for creating a snap package of openMVG.
See https://snapcraft.io for more information.

This is the recipe for creating a snap package of openMVG.
See https://snapcraft.io for more information.
@pmoulon
Copy link
Member

pmoulon commented Feb 10, 2017

Thank you for this contribution.
It's nice to have it, I would like to request you to consider also install the sensor_width_camera_database.txt file.
Here the install rule used in the CMake configuration
https://github.com/openMVG/openMVG/blob/master/src/openMVG/exif/sensor_width_database/CMakeLists.txt#L8
Adding a single copy instruction must do the trick.

@mardy
Copy link
Contributor Author

mardy commented Feb 10, 2017 via email

@mardy
Copy link
Contributor Author

mardy commented Feb 10, 2017

I've updated the branch and added the camera DB file to the package.

@pmoulon
Copy link
Member

pmoulon commented Feb 10, 2017

Thank you very much, will check it and merge it to develop for the next release.

@mardy
Copy link
Contributor Author

mardy commented Feb 12, 2017

Hi Pierre! Just in case you are wondering how to test the package: make sure you have the snap command available; if not, you can follow these instructions.
Once you have snap installed, you can just run

snap install openmvg

And the package will be installed. Note that in order to avoid namespace issues, all command are prefixed with openmvg; so, for example, in order to run GlobalSfM you'll actually need to type

openmvg.GlobalSfM

On the positive side, one can simply type openmvg. and then press TAB twice in the terminal to see the whole list of available OpenMVG tools.

@rjanvier
Copy link
Member

@mardy the cmake script detect cpu instructions (SSE, AVX etc) how this his handled by canonical buildbots?

@mardy
Copy link
Contributor Author

mardy commented Feb 12, 2017 via email

@rjanvier
Copy link
Member

https://github.com/openMVG/openMVG/blob/master/src/cmakeFindModules/FindSSE.cmake
I can't check for now but maybe @pmoulon can confirm you.

@pmoulon
Copy link
Member

pmoulon commented Feb 14, 2017

Here the line that I call
https://github.com/openMVG/openMVG/blob/master/src/CMakeLists.txt#L93

Since I do not call AutodetectHostArchitecture, I don't think that fancy CPU support is enable by now.

Default setting must just enable native build and SSE extension if the CPU is capable (SSE is pretty common now on every computer).

Since the project is working fine on ARM target too, I think the cmake detection is working right.

@mardy I think the best would be to put the snap file into a dir called ./dist and certainly that a tiny readme.md would be nice in order to explain to people how to create/obtain/How to use the snap.

@rjanvier
Copy link
Member

I agree @pmoulon, but since snaps are build on a buildfarm (for users that don't build them themself) this may lead to non reproductible builds. Worst, binary could be unusable and crash on host machine that do not support these instructions. Since you only use SSE2 it should be ok everywhere (more specially on 64 bits) now but I don't know how it's handled at third parties level (Eigen, Ceres, vlfeat...)

@mardy
Copy link
Contributor Author

mardy commented Feb 26, 2017

Hi! I moved the file as requested, added a README, and updated the recipe to include a couple of useful python scripts.

@pmoulon pmoulon added this to the Putative V1.2 milestone Feb 27, 2017
@mardy
Copy link
Contributor Author

mardy commented Feb 28, 2017

It seems the binary I've built indeed doesn't work on all machines (I've built it on an AMD CPU, and now I see that all programs crash at start on an Intel i7 ("illegall instruction").
This is the 'flags' line from my /proc/cpuinfo:

flags		: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf pni dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16 xtpr pdcm sse4_1 sse4_2 popcnt lahf_lm tpr_shadow vnmi flexpriority ept vpid dtherm ida

Do I need to explicitly disable AVX when building? Could that be the cause of the error?

@pmoulon
Copy link
Member

pmoulon commented Feb 28, 2017

Adding -DTARGET_ARCHITECTURE=generic to the cmake command line should do the trick.
Can you try it?

@mardy
Copy link
Contributor Author

mardy commented Feb 28, 2017

Hi Pierre! Thanks, that option helps :-)
I pushed a new commit which adds that option. Out of curiosity, how hard would it be to detect AVX at runtime?

@pmoulon
Copy link
Member

pmoulon commented Feb 28, 2017

Detecting AVX at runtime is not too hard.
You can do something similar to that 6447431 (here its sse4.2 check)

See https://insufficientlycomplicated.wordpress.com/2011/11/07/detecting-intel-advanced-vector-extensions-avx-in-visual-studio/

@pmoulon
Copy link
Member

pmoulon commented Mar 3, 2017

Thanks,
Merged by 49e7a16

@pmoulon pmoulon closed this Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants