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

Make Mantid work on windows with new setup tools versions #33541

Closed

Conversation

DannyHindson
Copy link
Contributor

@DannyHindson DannyHindson commented Feb 10, 2022

Description of work.

The setuptools develop mode previously allowed workbench to run successfully by providing .egg-link files such as MantidWorkbench.egg-link in the bin\<config> directory which helped locate python packages in the source code directories. The .egg-link files aren't supported any more if using python 3.8 and setuptools>=49.0.0 so an alternative mechanism is needed.

This PR sets up two sitecustomize.py files that sort this out:
a) a sitecustomize.py file in the bin/<config> directory that sets up bin/<config> as a site directory. This causes the .pth files in here to be read and achieve the same thing as the .egg-link files
b) a sitecustomize.py file in the python site-packages directory to ensure that the file in a) is found. The cwd is added to the path

Notes:
The file in a) has actually been called sitecustomize_mantid.py to ensure both files are found and one doesn't hide the other. Python only supports finding x1 sitecustomize.py file and the search order through site-packages and bin/<config> directory isn't the same for all methods of starting up workbench

There was already some cmake logic to create the file in a) but I don't think it was being used in most situations. The .egg-link files were being relied upon instead.

The sitecustomize.py file in b) already existed for Windows users and was used to set up some paths to the 3rd party libraries. It was stored in the third-partymsvc repository. Since it's needed for all users with python3.8 and setuptools>=49.0.0 it makes more sense to create it in cmake

To test:

I think this needs testing on various platforms so may leave this until the next maintenance sprint:

Upgrade setuptools to a new version: pip install "setuptools==49.0.0"
Make a note of the original setuptools version reported in the message so you can switch back at the end
Run cmake
Build the workbench target which will generate a new shim file in the bin/<config> directory. This is called workbench-script.pyw on Windows
Try launching workbench using the following methods:
a) from PyCharm using the Run, Run and Run, Debug methods
b) On Windows, using the MantidWorkbench target

Fixes #33118 .

This does not require release notes because there's no functional change to users


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

To avoid it hiding the sitecustomize.py in the python site-packages
directory on Windows
Also add comments to explain what's going on
Copy link
Member

@martyngigg martyngigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we need to remove the sitecustomize.py in the third party repo now?

"
)

file(WRITE ${PYTHON_SITE_PACKAGES}/sitecustomize.py "${SITE_CUSTOMIZE_CONTENT}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I understand the process - is this essentially here to get the cwd onto the sys.path?

Could I ask where this goes on Linux? The site-packages from the system would not be writable I don't think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the content of this sitecustomize.py file that applies to all platforms is aimed at getting the cwd onto the sys.path. If site-packages is not writable on some platforms then that is a problem :(

py_exe_dir, # required for C++ tests that embed Python to find Python DLL
thirdparty_bin,
thirdparty_bin + \"\\\\mingw\",
thirdparty_lib + \"\\\\qt4\\\\bin\",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the Qt4 ones from here?

@DannyHindson
Copy link
Contributor Author

Will we need to remove the sitecustomize.py in the third party repo now?

Yes we'd need to do that as well although feels like I'm probably a little way off doing that level of polishing up on this yet. There's a fair amount of testing needs to be done on the various platforms and perhaps the approach itself needs reviewing if the system site-packages directory isn't writable on all platforms (as discussed in separate comment).

@martyngigg
Copy link
Member

@DannyHindson I'm expecting the conda switch might make this less of a requirement. Should we close this and revisit after this release?

@DannyHindson
Copy link
Contributor Author

@DannyHindson I'm expecting the conda switch might make this less of a requirement. Should we close this and revisit after this release?

Yes that's fine. I think I'd got a bit stuck with this given that my most recent solution was based on site-packages being writable

@sf1919 sf1919 deleted the 33118_MakeMantidWorkOnWindowsWithNewSetupTools2 branch July 1, 2024 12:31
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.

Mantid doesn't work with latest setuptools on Windows
2 participants