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

mod: Add post installation fix of OpenCV pathes instead of hot fixing #1628

Merged
merged 7 commits into from
Apr 6, 2023

Conversation

coumbsek
Copy link

@coumbsek coumbsek commented Apr 3, 2023

If ODM is install in a folder where the running user does not have writing permession it fails.

This is due to the hot fixing of the pyvenv.cfg and cv2 extension configs at each run.

Only read and execution permissions should be sufficient to run ODM, I think this is best pratice, at least under Windows not to write file in the installation folder during runs.

In order to achieve this I propose to transform the hot fixing to a cold fixing done once at install.
For this I introduce a bat file that is run by the setup after completing the installation process. This bat does exactly what the hot fixing was doing but once and for all.

Hope it will pass the tests and be accepted.

@pierotofy
Copy link
Member

pierotofy commented Apr 3, 2023

Hey @coumbsek 👋 thanks for the PR!

I think this is a good approach and yes I agree that ideally we should not write to the installation folder.

There's a few other parts of the codebase that currently write to the installation folder (the AI model download code in particular), perhaps a few other places, so I think this should be included as part of a larger work to make the program work without writes to the install folder.

This means it might take a bit before I can merge this in master. Help to fix the other parts the codebase that currently write to the install folder would speed this up of course.

@coumbsek
Copy link
Author

coumbsek commented Apr 4, 2023

Ok, I just looked at the ai model download.

There is two cases of writing in the installation folder :

  1. What can be done once and for all at setup
  2. What must be done at runtime each time.

The case of ai model currently falls inside the second case because it is an "optional" feature and has such is only downloaded the first time we need it at runtime.
There is only two options that I see to "fix" this case :

  • Add the models as optional features during the setup and then only proceed if model has been installed
  • Else If model is not present inside installation folder, :
    • If user has permission to write to the installation folder no issue we do what we do currently
    • If user has no permission to write to the installation folder ask for elevated permissions (UAC under windows for instance), if permission is not given (or automatically) fallback to download the model each time inside the project folder instead (always have right to wright there but implies that we redownload-it each time and we do not want that)

A mitigation would be to use the user folder (like AppData under windows) to download the models, here we have right wrights and we would not need to download it for each dataset but only for each user calling ODM.

What to you think ?

@pierotofy
Copy link
Member

A mitigation would be to use the user folder (like AppData under windows) to download the models

I think this would be the way to go.

@coumbsek
Copy link
Author

coumbsek commented Apr 5, 2023

Havinga look at what we can do with Innosetup I think an even better case would be to use an ODM folder inside ProgramData and make it writable for all users during setup. I think this is exactly what ProgramData is about.

Regarding the others platforms (linux, mac) I have no idea of what is the equivalent and how we should manage it.

@pierotofy
Copy link
Member

Linux/Mac behavior can probably be left as-is for the time being.

@coumbsek
Copy link
Author

coumbsek commented Apr 5, 2023

Ok, so it should be fixed for ai models. Do you see some other places where I should look at to fix it the same way ?

@pierotofy
Copy link
Member

I think there's just the __pycache__ stuff (which I should have taken care of). I'll run some tests before merging but it's looking good. Thanks!

@coumbsek
Copy link
Author

coumbsek commented Apr 5, 2023

I just realise it was missing a variable declaration in the winpostinstall.bat so the cv2 package was not initialising properly.

set SBBIN=%ODMBASE%SuperBuild\install\bin

@pierotofy pierotofy merged commit 2361fce into OpenDroneMap:master Apr 6, 2023
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.

2 participants