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

Fix strange python launcher error when folder has spaces #1627

Merged
merged 8 commits into from
Apr 1, 2023

Conversation

HeDo88TH
Copy link
Contributor

@HeDo88TH HeDo88TH commented Mar 29, 2023

This PR fixes the behavior of innosetup to use the windows short path when it contains spaces.
The issue seems to be related to pypa/setuptools#216 and several others. I tested the same exact setup with newer python versions and they work fine. Unfortunately we are stuck with python 3.8 due to breaking changes in newer versions.

Bonus: added vcpkg, venv, python38, dist and innosetup folders to gitignore.

Closes #1569

@pierotofy
Copy link
Member

pierotofy commented Mar 29, 2023

Thanks. I'm a bit hesitant to accept this as a solution, it's my understanding that you're simply forcing users to install ODM to a path without spaces (via shortpath)? I also have concerns about that "Delete Folder" statement (potentially dangerous) and the ASCII encoding requirement (will this even work with non-ASCII characters in the path?)

If this issue was fixed in newer versions of Python, can't we just backport the patch?

For example, have you attempted to apply pypa/setuptools#2697 ?

@HeDo88TH
Copy link
Contributor Author

We abandoned the short path workaround in favour of merging the folders python38 and venv\Scripts.
It works and test pipeline runs!

@pierotofy
Copy link
Member

pierotofy commented Mar 29, 2023

Need to wait for https://gitlab.com/gitlab-org/gitlab/-/issues/402616 to test this (it's causing the ODM builds to fail).

2023-03-29T18:11:09.9728890Z     Performing download step (download, verify and extract) for 'ext_eigen'
2023-03-29T18:11:10.0050628Z     -- Downloading...
2023-03-29T18:11:10.0053940Z        dst='D:/a/ODM/ODM/SuperBuild/build/mvstexturing/elibs/ext_eigen/src/eigen-3.3.2.tar.gz'
2023-03-29T18:11:10.0054871Z        timeout='none'
2023-03-29T18:11:10.0056023Z        inactivity timeout='none'
2023-03-29T18:11:10.0057117Z     -- Using src='https://gitlab.com/libeigen/eigen/-/archive/3.3.2/eigen-3.3.2.tar.gz'
2023-03-29T18:11:10.8564153Z     -- verifying file...
2023-03-29T18:11:10.8565675Z            file='D:/a/ODM/ODM/SuperBuild/build/mvstexturing/elibs/ext_eigen/src/eigen-3.3.2.tar.gz'
2023-03-29T18:11:10.8657097Z     -- MD5 hash of
2023-03-29T18:11:10.8657908Z         D:/a/ODM/ODM/SuperBuild/build/mvstexturing/elibs/ext_eigen/src/eigen-3.3.2.tar.gz
2023-03-29T18:11:10.8659324Z       does not match expected value
2023-03-29T18:11:10.8660476Z         expected: '02edfeec591ae09848223d622700a10b'
2023-03-29T18:11:10.8661183Z           actual: '7b8ab470dbf9d22bf4adef60f9f8f4f2'

Not related to the changes here.

@HeDo88TH
Copy link
Contributor Author

Need to wait for https://gitlab.com/gitlab-org/gitlab/-/issues/402616 to test this (it's causing the ODM builds to fail).

They messed up some hashes 😢

@pierotofy
Copy link
Member

Everything checks out, thanks!

@pierotofy pierotofy merged commit 91201d5 into OpenDroneMap:master Apr 1, 2023
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.

Windows installation in non-default path causes error
2 participants