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 Windows regression in omero_ext.path #402

Merged
merged 2 commits into from
Mar 25, 2024
Merged

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Mar 22, 2024

135a712 partly reverts commit efbb1a4 which disabled the CI builds on Windows 2022 as Ice 3.6.5 requires additional patches for building and led to some regression in the Python 3.12 support work. This commit reintroduces Windows 2019 as a target operating system with a Python version (3.9) with > 1 year of support. It also upgrade the underlying GitHub actions for Node.js 20.

8f16016 reintroduces the try/except block removed in f054810 which led to regression on Windows systems where pwd is not installed.

Usin a Windows system using OMERO.py 5.19.0, any script or command e.g. omero login trying to import omero_ext.path should fail with a ModuleNotFoundError. With this PR, the same script or command should complete successfully.

Given it is a regression, proposing to schedule this for an emergency 5.19.1 patch release once testing is complete.

/cc @mabruce @erindiel

This partly reverts commit efbb1a4
which disabled the CI builds on Windows 2022 as Ice 3.6.5 requires
additional patches for building and led to some regression in the
Python 3.12 support work.

This commit reintroduced Windows 2019 as a target operating system
with a Python version (3.9) with >1 year of support. It also upgrade
GitHub actions for Node.js 20.
The try/except block was removed in f054810
which led to regression on Windows systems where pwd is not installed.
@jburel
Copy link
Member

jburel commented Mar 22, 2024

Access to windows can be limited for testing, we should probably re-activate the build on windows via GHA and disable the table tests which were the source of the problem last time

@sbesson
Copy link
Member Author

sbesson commented Mar 22, 2024

Access to windows can be limited for testing, we should probably re-activate the build on windows via GHA and disable the table tests which were the source of the problem last time

I don't understand, isn't that exactly what this PR does?

@jburel
Copy link
Member

jburel commented Mar 22, 2024

Sorry I missed the line when checking the action.

@pwalczysko
Copy link
Member

@sbesson I think I have a Windows testing system (MyDesktop provided by UoD) where I can install from conda (ie. I can successfully install omero-py release as

conda create -n myenv -c conda-forge python=3.10 omero-py and it works and I can log in to outreach server.
The problem is to test this PR. The pip install -e . fails with complaint that the machine actively rejected connection. Although I have git installed and this PR checked out, this is still a blocker.

So I think if it is of interest, I could test an artifact from, say conda-forge which you could push in from this PR ?

But if the GHA test is sufficient, then we can leave it too.

@sbesson
Copy link
Member Author

sbesson commented Mar 22, 2024

The problem is to test this PR. The pip install -e . fails with complaint that the machine actively rejected connection. Although I have git installed and this PR checked out, this is still a blocker.

Do you have a more precise warning of what is actually failing? You don't need -e as it's only for using projects in editable mode.

So I think if it is of interest, I could test an artifact from, say conda-forge which you could push in from this PR ?

Is there some pre-built mechanism to do that? I could certainly build a wheel package from this PR if that's useful.

@mabruce
Copy link

mabruce commented Mar 22, 2024

I checked out this PR locally and installed (pip install .) into a clean virtual environment (Python 3.10, Windows 10). omero login works as expected with this PR, and fails as reported with 5.19.0.

@pwalczysko
Copy link
Member

pwalczysko commented Mar 25, 2024

The problem is to test this PR. The pip install -e . fails with complaint that the machine actively rejected connection. Although I have git installed and this PR checked out, this is still a blocker.

Do you have a more precise warning of what is actually failing? You don't need -e as it's only for using projects in editable mode.

So I think if it is of interest, I could test an artifact from, say conda-forge which you could push in from this PR ?

Is there some pre-built mechanism to do that? I could certainly build a wheel package from this PR if that's useful.

@sbesson thanks, on a second look, I think already my git clone cmd failed (which I did not see on Friday, sorry)

base) H:\test>git clone https://github.com/ome/omero-py.git
Cloning into 'omero-py'...
remote: Enumerating objects: 42991, done.
remote: Counting objects: 100% (1072/1072), done.
remote: Compressing objects: 100% (375/375), done.
remote: Total 42991 (delta 702), reused 1026 (delta 677), pack-reused 41919
Receiving objects: 100% (42991/42991), 11.90 MiB | 8.79 MiB/s, done.
Resolving deltas: 100% (33082/33082), done.
error: unable to create directory for '//dundee.uni/users/SLSC/PWalczysko/test/omero-py/.git/logs/refs/remotes/origin/HEAD': No such file or directory
fatal: update_ref failed for ref 'HEAD': cannot lock ref 'HEAD': unable to create directory for //dundee.uni/users/SLSC/PWalczysko/test/omero-py/.git/HEAD
Unlink of file 'omero-py/.git/objects/pack/pack-d46e7a810a1309da9248a73def67bdf580caec24.idx' 

Further, my experience with the fresh release install of omero-py using anaconda on this Windows VM is that the login to omero works.
Hence, I cannot repeat the original bug.

I will approve, this ^^^ seems just too hard to crack.

@sbesson sbesson merged commit 815874b into ome:master Mar 25, 2024
9 checks passed
@sbesson sbesson deleted the win_regression branch May 15, 2024 12:39
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.

4 participants