Fix bugs and add support for UNC paths on Windows. #901

acolwell · 2023-07-03T20:07:17Z

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:

I've followed the contributing guidelines to the best of my understanding
I've read and understood the contributing guidelines
I've formatted my code according to Natron's code style
I've searched the pull requests tracker to ensure that this PR is not a duplicate

PR Description

What type of PR is this? (Check one of the boxes below)

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (non-breaking change which does not add functionality nor fixes a bug but improves Natron in some way)
Breaking change (fix or feature that would cause existing functionality to change)
My change requires a change to the documentation
- I have updated the documentation accordingly

What does this pull request do?

This pull request fixes a variety of issues with network share paths on Windows in the file dialogs. (#896 )

The change is split into 2 pieces. The first commit mainly just cleans up the code a little bit by introducing some helper functions, removing duplicate code, and introducing unit tests for key FileSystemModel functionality related to paths. No major behavior changes are in this commit. It is mainly just moving stuff around and trying to make the code a little more consistent and readable.

The second commit contains the bug fixes and new logic to support network paths on Windows. It fixes the major issues with navigating network shares and should address all the issues mentioned in (#896).

Have you tested your changes (if applicable)? If so, how?

Yes. I created unit tests to verify the key path manipulation functionality in FileSystemModel. I also built a Windows version of Natron locally and did a bunch of manual tests on the file dialog with network shares. The behavior is MUCH better than it was and the network shares essentially behave just like drives.

- Added helper functions to reduce redundant code, improve readability, and help clarify intent. - Added getSplitPath() and cleanPath() to FileSystemModel to make it easier to test and extend this functionality. - Added unit tests for some FileSystemModel functions to document existing behavior and make it easier to see how behavior changes with bug fixes. - Deleted unused code.

- Added code to detect network share paths on Windows and treat make them behave like drives. - Fixed a bug where paths typed in with lower case drive letters would cause an upper case drive AND a lower case drive entry to appear in the "My Computer" view. A similar thing would happen for network share hostnames. Added logic to upper case the drive letter and hostnames as part of the "path cleanup" logic. This causes the paths to map to single entries in the "My Computer" view. - Fixed an infinite loop during directory restore when a non-existant network share was specified in a previous Open File dialog instance. - Fixed "up" button so that it behaves more consistently and properly handles network share paths. - Fixed invalid memory access for files that start with a '.' - Fixed a few places where code was using a URL path instead of converting the URL to a local path string like the vast majority of the code. This made it possible to properly view the root directory of network shares. - Updated unit tests to reflect changes in path handling behaviour.

devernay · 2023-09-05T08:53:59Z

Does it fix some or all of the following:

Can another windows developer review this? @rodlie ? This looks OK to me

devernay · 2023-09-05T09:50:08Z

@acolwell I now have a windows 11 laptop with enough disk space to try things out. Can you confirm that the instructions using MSYS2 from https://github.com/NatronGitHub/Natron/blob/RB-2.5/INSTALL_WINDOWS.md are the way to go? Do you have any plans to do a VS build? Can you please check if the instructions look ok?
Or should I prefer this sequence of instructions to install the dependencies:

mkdir ${GITHUB_WORKSPACE}/natron_pacman_repo
cd ${GITHUB_WORKSPACE}/natron_pacman_repo
wget https://github.com/NatronGitHub/Natron/releases/download/windows-mingw-package-repo/natron_package_repo.zip
unzip natron_package_repo.zip
NATRON_REPO_PATH=`cygpath -u $GITHUB_WORKSPACE`
echo -e "#NATRON_REPO_START\n[natron]\nSigLevel = Optional TrustAll\nServer = file://${NATRON_REPO_PATH}/natron_pacman_repo/\n#NATRON_REPO_END" >> /etc/pacman.conf
pacman -Syl natron
pacman -S --needed --noconfirm mingw-w64-x86_64-natron-build-deps-qt5

devernay · 2023-09-05T13:37:59Z

I installed msys2 using choco install msys2 ~~but uname -s gives MSYS_NT-10.0-22621 which is not handled by the build script. Did I do something wrong?~~
The console has then to be launched using "mingw64", not "msys2"

acolwell · 2023-09-05T17:54:44Z

Does it fix some or all of the following:

(Bug): UNC Path Support in File Dialogs #896

Bad network paths on symlink provided to OCIO. #236

Can another windows developer review this? @rodlie ? This looks OK to me

It fixes #896, but I haven't looked into #236. I'll try to look into that bug in the next day or two. Assuming it goes through the same path abstractions that I fixed, it should work.

acolwell · 2023-09-05T18:34:42Z

@acolwell I now have a windows 11 laptop with enough disk space to try things out. Can you confirm that the instructions using MSYS2 from https://github.com/NatronGitHub/Natron/blob/RB-2.5/INSTALL_WINDOWS.md are the way to go? Do you have any plans to do a VS build? Can you please check if the instructions look ok? Or should I prefer this sequence of instructions to install the dependencies:
mkdir ${GITHUB_WORKSPACE}/natron_pacman_repo
cd ${GITHUB_WORKSPACE}/natron_pacman_repo
wget https://github.com/NatronGitHub/Natron/releases/download/windows-mingw-package-repo/natron_package_repo.zip
unzip natron_package_repo.zip
NATRON_REPO_PATH=`cygpath -u $GITHUB_WORKSPACE`
echo -e "#NATRON_REPO_START\n[natron]\nSigLevel = Optional TrustAll\nServer = file://${NATRON_REPO_PATH}/natron_pacman_repo/\n#NATRON_REPO_END" >> /etc/pacman.conf
pacman -Syl natron
pacman -S --needed --noconfirm mingw-w64-x86_64-natron-build-deps-qt5

So that page could probably use an update. My "from scratch" workflow roughly matches the windows installer build GitHub action.

Install mingw64 via the installer on https://www.msys2.org/
Open a mingw64 console
pacman -S git mingw-w64-x86_64-wget unzip
Install Natron MINGW pacman repo (https://github.com/NatronGitHub/Natron/releases/tag/windows-mingw-package-repo)
Install Inno Setup (https://jrsoftware.org/isdl.php)
mkdir ~/natron_build
Checkout Natron somewhere (e.g. ~/src/Natron)
cd ~/src/Natron/tools/jenkins
WORKSPACE=~/natron_build BUILD_NAME=natron BUILD_NUMBER=1 BITS=64 NATRON_LICENSE=GPL GIT_URL=https://github.com/NatronGitHub/Natron.git GIT_URL_IS_NATRON=1 SNAPSHOT_BRANCH=RB-2.5 QT_VERSION_MAJOR=5 DISABLE_BREAKPAD=1 NOUPDATE=1 MKJOBS=$(nproc) ./launchBuildMain.sh
Once this finishes an installer zip is located in ~/natron_build/builds_archive/natron/1
Unzip the file in that directory and you'll have a full Natron install.

Once I have this fully built install, my incremental workflow is the following:

pacman -S --needed mingw-w64-x86_64-cmake mingw-w64-x86_64-ninja
Goto Natron source directory (e.g. src/Natron)
mkdir build; cd build
cmake ..
Build: ninja
Copy to Installer dir. : (INSTALLER_ZIP=$(ls ~/natron_build/builds_archive/natron/1/Natron-*Windows-x86_64.zip) ; cp -v App/Natron.exe Tests/Tests.exe $(dirname ${INSTALLER_ZIP})/$(basename ${INSTALLER_ZIP} .zip)/bin/)
Run Natron or Tests in installer dir
Make changes and goto step 5.

Ultimately I'd like to get to a place where I don't need to do a full installer build. Downloading prebuilt test plugin zips like the Natron CI action does is probably the way forward, but I haven't done that yet. I'd also like to get to a place where we aren't using qmake for the installer and cmake for my incrementals. I've just found that the cmake/ninja combo seems faster than the qmake path when doing development. I start with the installer build because that seemed like the "official" way to build releases, but the cmake path seems close enough at this point that I can rely on it for dev purposes.

I have not tried to do a VS build yet especially since we have to build Natron specific versions of certain libraries and at least right now that is easier to do/maintain via pacman packages. Long term, I think it might be possible & easier if we eventual migrated to something like Conan (https://conan.io/center) to build all the things. Over the last few months I have tinkered with that and have almost built all of the plugin repos on all platforms with Conan. At some point I'd like to try getting Natron to build as well, but I just don't have the time/energy to tackle that one yet. For now I've just been focusing on trying to get the existing build system working, repeatable, and a little easier to maintain.

I hope this helps.

devernay

LGTM, please close the corresponding bug(s)

acolwell · 2023-09-11T15:15:10Z

Thanks for the reviews. :)

Al3145 · 2024-08-03T02:29:13Z

hello @acolwell, is your build process here still working or has there been changes?

acolwell · 2024-08-03T02:52:40Z

hello @acolwell, is your build process here still working or has there been changes?

Yes. This is still the way I work today. The only difference is that the current branch is RB-2.6 and not RB-2.5

Al3145 · 2024-08-03T03:04:11Z

Gotcha. Thank you!

acolwell force-pushed the fix_windows_network_shares branch from b87a400 to fc4298a Compare July 4, 2023 22:22

acolwell force-pushed the fix_windows_network_shares branch from fc4298a to e12e38d Compare July 15, 2023 21:00

acolwell added 2 commits August 21, 2023 18:42

acolwell force-pushed the fix_windows_network_shares branch from e12e38d to 238eacd Compare August 22, 2023 01:43

devernay mentioned this pull request Sep 5, 2023

GitHub Action for building Windows installers #912

Merged

10 tasks

devernay self-requested a review September 11, 2023 12:51

devernay approved these changes Sep 11, 2023

View reviewed changes

acolwell merged commit cc07dc6 into NatronGitHub:RB-2.5 Sep 11, 2023

acolwell deleted the fix_windows_network_shares branch September 11, 2023 15:15

This was referenced Sep 11, 2023

(Bug): UNC Path Support in File Dialogs #896

Closed

I cannot build Natron with instructions, mingw-w64-shiboken prevented me.(Bug): #921

Closed

acolwell mentioned this pull request Oct 17, 2023

Fix asserts and build buster in debug build. #930

Merged

10 tasks

This was referenced Jun 20, 2024

Comment out check for debug flag build type. #972

Merged

(Bug): CMake debug build fails at linking. #973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugs and add support for UNC paths on Windows. #901

Fix bugs and add support for UNC paths on Windows. #901

acolwell commented Jul 3, 2023

devernay commented Sep 5, 2023

devernay commented Sep 5, 2023 •

edited

Loading

devernay commented Sep 5, 2023 •

edited

Loading

acolwell commented Sep 5, 2023 •

edited

Loading

acolwell commented Sep 5, 2023 •

edited

Loading

devernay left a comment

acolwell commented Sep 11, 2023

Al3145 commented Aug 3, 2024

acolwell commented Aug 3, 2024

Al3145 commented Aug 3, 2024

Fix bugs and add support for UNC paths on Windows. #901

Fix bugs and add support for UNC paths on Windows. #901

Conversation

acolwell commented Jul 3, 2023

PR Description

devernay commented Sep 5, 2023

devernay commented Sep 5, 2023 • edited Loading

devernay commented Sep 5, 2023 • edited Loading

acolwell commented Sep 5, 2023 • edited Loading

acolwell commented Sep 5, 2023 • edited Loading

devernay left a comment

Choose a reason for hiding this comment

acolwell commented Sep 11, 2023

Al3145 commented Aug 3, 2024

acolwell commented Aug 3, 2024

Al3145 commented Aug 3, 2024

devernay commented Sep 5, 2023 •

edited

Loading

devernay commented Sep 5, 2023 •

edited

Loading

acolwell commented Sep 5, 2023 •

edited

Loading

acolwell commented Sep 5, 2023 •

edited

Loading